React: Warn when setState is a function that doesn't return

[sw-yx]

Do you want to request a feature or report a bug?

I am proposing adding a warning in development.

What is the current behavior?

when I do this.setState(({ bool }) => { bool: !bool }); this is valid javascript but is meaningless in React. ESLint:no-label helps to catch this but we can probably do one better by building a warning into dev-mode React itself.

sandbox demo: https://codesandbox.io/s/xopj5nx07o

What is the expected behavior?

warn when a function is applied to setState that returns undefined. if the user wants to indicate nothing changed they should return null.

[gaearon]

I wish we did this earlier. Might be too late now because there’s probably a lot of code using this pattern. #strictmodeonly?

[tsiq-swyx]

strictmodeonly works for me.

i'm not sure i understand, why would there be a lot of code using this pattern? accidental no-op is far more likely than intentional no-op?

also we'd just be doing a warning, i reckon if people think they know what they're doing we can let them ignore the warning. i dunno, is that inline with react's warning philosophy?

[alexkrolick]

i'm not sure i understand, why would there be a lot of code using this pattern? accidental no-op is far more likely than intentional no-op?

Probably something like:

this.setState(state => { if (condition) return { foo: !state.foo } }) // no else

[gaearon]

i'm not sure i understand, why would there be a lot of code using this pattern? accidental no-op is far more likely than intentional no-op?

I don't think it's necessarily more likely.

also we'd just be doing a warning, i reckon if people think they know what they're doing we can let them ignore the warning.

No, if we add a warning, we don't want people to ignore it. This devalues warnings.

[sw-yx]

strict mode it is

[aweary]

@sw-yx do you want to submit a PR for this?

[tsiq-swyx]

yup i will attempt it this weekend if thats ok?

On Tue, Jun 26, 2018 at 10:48 PM Brandon Dail [e-mail]
wrote:

@sw-yx https://github.com/sw-yx do you want to submit a PR for this?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react/issues/13111#issuecomment-400525534,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AiT1gtLGCY5yqTVilBJ9MI8KAYdTweB-ks5uAvJkgaJpZM4U4csG
.

[sw-yx]

ok i had a rough day and felt like i needed the psychological pickup so i'm going to try this now. for anyone following along i am marking out the thought process.

• fork facebook/react to sw-yx/react. open a new addWarning branch.
• search /packages/react for setState
• realize that nothing of note is actually done in packages/react
• eventually find /packages/react-reconciler/src/ReactFiberClassComponent where I find enqueueSetState(inst, payload, callback).
• this is progress! the first argument of setState is called a "payload".
• search the rest of the codebase for ".payload" and find ReactUpdateQueue.js where I finally find "payload.call" (i could have skipped all this ceremony if i had known to search for "Updater function" which is commented everywhere setState is executed
• unsure whats the difference between update.tag == ReplaceState or UpdateState. but i finally have the site where i can insert my warning. yay! 10% done.

after adding my warning:

        if (__DEV__) {
          if (StrictMode && partialState === undefined) {
            // Warn if partialState is undefined
            // dev likely forgot to return in setState
            // encourage returning null for no-op in strict mode
            warning(
              false,
              'A setState updater function was called that returned undefined. ' +
                'Check if you forgot to return a value in your setState updater ' +
                'function. If a no-op was intended, return null.'
            );
          }
        }

• realize that warning is the old way of warning things, there's this "Strict Mode Warnings" that are being done where they seem to track warnings that have already been issued (i guess so as not to spam warnings). this is in ReactStrictModeWarnings.js
• realize that my kind of warning doesnt have any place in ReactStrictModeWarnings, so rather than add a whole new category to ReactStrictModeWarnings i will leave it as is unless powers that be decide this is necessary
• time to write tests!
• find a test called it('can use updater form of setState' inside ReactIncremental-test.internal.js. basically you want to look for the absolute simplest test that is close to your particular piece of code, so you can copy it and also colocate it. i have no idea what ReactIncremental is, looks like some incremental rendering thingy based on the other tests.
• copy it
• comment out the irrelevant bits, and intentionally return undefined for updater in the test, add StrictMode
• how to test a warning? look for similar warnings in the same file, fortunately there is one that basically says to use expect(ReactNoop.flush).toWarnDev
• ok thats the test done. now to run it. the docs say yarn test --watch TestName is handy so thats what i do: yarn test --watch ReactIncremental-test.internal.js
• my tests pass on first try, thats not necessarily good. you want to make sure your tests fail when they are supposed to fail. Replace React.StrictMode with React.Fragment
• tests still pass. uh oh.
• return null in my updater
• test fails! yes!! Expected warning was not recorded: "A setState updater function was called that returned undefined. Check if you forgot to return a value in your setState updater function. If a no-op was intended, return null."
• good. that's what a broken test should look like. it means that my StrictMode condition isn't doing anything. Either my test is wrong or my warning is wrong.
• revert the updater to return undefined
• now I am still testing this without React.StrictMode. the test SHOULD NOT pass but it does.
• every other test tests in React.StrictMode straightforwardly, which means my warning implementation is probably the culprit.
• i use StrictMode from ./ReactTypeOfNode. it's a constant, not a derived boolean. i was misusing it.
• looks like i have to call workInProgress.mode & StrictMode instead.
• cool! next step according to the docs: yarn test-prod
• yarn prettier
• yarn linc (better than yarn lint apparently)
• yarn flow dom (has some options to choose)
• git add (relevant files only, dont add yarn.lock) & `git commit -m "add warning for setState updater function returning undefined in strict mode"
• git push origin addWarning (i have this in an addWarning branch in my fork of React)
• open PR to facebook/react@master :)

[KiritoCheng]

I use with typescript ,now its's error "Cannot assign to 'state' because it is a constant or a read-only property."
I just write
constructor(props: any) {
super(props);
this.state = {
a:'a'
}
}

[tsiq-swyx]

sorry, i dont think this has anything to do with my PR. are you commenting in the right issue? also if you want help with React + Typescript please see https://github.com/sw-yx/react-typescript-cheatsheet

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale[bot]]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!