Add warning to prevent setting this.state to this.props referentially #11658

veekas · 2017-11-25T04:41:48Z

Adds warning to prevent setting this.state = props. Closes #11593.

Passed all tests with yarn test
Ran yarn prettier
Ran yarn lint
Ran yarn flow
Completed CLA

Thanks for your help on this first PR, @sw-yx and @gaearon.

gaearon · 2017-11-25T13:35:35Z

Most people don't know what "referentially" means.
We also need to include the component name.

I would reword this as:

It looks like the %s component contains a line like this in its constructor:

this.state = props;

This is not recommended because any further updates to props won't be reflected in the state. In most cases, you don't need to keep state and props in sync. Instead, use the props directly. If you need to calculate something from the props, do it during the rendering. If you need to share the state between several components, move it to their closest common ancestor, and pass it down as props to them.

veekas · 2017-11-25T21:43:57Z

Updated to include the new warning message and component name. Ran test/prettier/linc/flow again.

aweary · 2017-11-28T02:12:24Z

packages/react-reconciler/src/ReactFiberClassComponent.js

@@ -465,6 +465,19 @@ export default function(
    instance.refs = emptyObject;
    instance.context = getMaskedContext(workInProgress, unmaskedContext);

+    const noSetStatesFromProps = !(instance.state === instance.props);
+    warning(
+      noSetStatesFromProps,


I know this is consistent with the rest of the warnings, but I wonder if we should just inline noSetStatesFromProps and avoid the whole negation thing?

warning( instance.state !== instance.props, `...` )

That looks better to me, as well. ~~Should I make the edit?~~ Edit: Made the edit and pushed a new commit.

the guidance is not clear here tbh. i've seen @gaearon also prefer warning(false,.... and doing all the logic in an if block. ¯_(ツ)_/¯

Typically we do warning(false, "") when we also need to execute some other code when issuing the warning, such as setting a flag so that the warning is deduped.

veekas · 2018-01-03T21:06:24Z

Hi @gaearon and @aweary, I wanted to check on this issue. Is there anything else you would like me to do to help close it?

gaearon · 2018-01-05T17:30:06Z

packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js

+    expectDev(console.error.calls.count()).toBe(1);
+    expectDev(console.error.calls.argsFor(0)[0]).toContain(
+      'It looks like the StatefulComponent component contains a line like this in its constructor: ' +
+        'this.state = props; ' +


Have you checked how this looks in a browser? Since you didn't add the linebreaks I assume it will look a bit confusing.

Good point. Pushed a new commit with line breaks before and after the code excerpt to increase readability.

veekas · 2018-03-16T19:40:44Z

Looks like CircleCI is failing due to the same CI/Docker issue (nodejs/docker-node#651) referenced by @bvaughn in #12392.

#!/bin/bash -eo pipefail
yarn install
/bin/bash: yarn: command not found
Exited with code 127

Lint, flow, and tests are passing locally. Anyway, ready for your review, @gaearon!

raunofreiberg · 2018-03-16T21:25:30Z

packages/react-reconciler/src/ReactFiberClassComponent.js

+        'something from the props, do it during the rendering. If you need to ' +
+        'share the state between several components, move it to their closest ' +
+        'common ancestor and pass it down as props to them.',
+      getComponentName(workInProgress) || 'Unknown',


Just a small nit: it would probably be a better idea to pass A component instead of Unknown if no component name is found.

Your example:

It looks like the Unknown component contains a line ...

This formatting seems quite odd to me. What the Unknown component? What if I in a rare case I have my own Unknown component and it's not the erroring one?

Suggestion:

A component contains a line ...

or if the component name exists

Foo contains a line like this...

This would also keep the handling of missing component names in error messages consistent. See: ReactFiberTreeReflection.js, ReactDOM.js

I'm inclined to agree, but as I searched through the code base I saw the getComponentName(*) || 'Unknown' pattern used 22 times while Component or A component were only used 7 and 8 times respectively. In fact, the most recent commits by @gaearon, @acdlite, and @bvaughn all use Unknown (#12028, #12359, #11455). I'll let one of them chime in about whether they think this should be changed.

It's true that "Unknown" is probably a more common fallback name, but it's not strictly necessary to use that pattern. That being said, I think this warning is a bit verbose and maybe tries to cover too broad a topic (e.g. sharing state between several components).

At first glance, I would prefer a shorter message, something like:

warning( instance.state !== instance.props, "%: It is not recommended to assign props directly to state. " + "Updates to props won't be reflected in state. " + "It's usually better to just use props directly.", getComponentName(workInProgress) || "Unknown" );

That being said, I realize this contradicts an earlier suggest from Dan so I'll defer if there are stronger preferences there.

veekas · 2018-04-02T22:50:39Z

It's been a couple weeks without comment, so I incorporated @bvaughn's and @raunofreiberg's input into the latest iteration. Thanks, you two, I think your edits make it more semantic and terse. Pinging @gaearon

bvaughn · 2018-04-03T04:24:56Z

packages/react-reconciler/src/ReactFiberClassComponent.js

+        "because updates to props won't be reflected in state." +
+        'In most cases, it is better to use props directly.',
+      getComponentName(workInProgress) || 'A component',
+    );


This needs to be within an if (__DEV__) check. Suggest using the one right below (line 711).

bvaughn · 2018-04-03T04:25:15Z

packages/react-reconciler/src/ReactFiberClassComponent.js

+    warning(
+      instance.state !== instance.props,
+      '%s: It is not recommended to assign props directly to state' +
+        "because updates to props won't be reflected in state." +


Need a trailing space (" ") here.

bvaughn · 2018-04-03T17:59:54Z

packages/react-reconciler/src/ReactFiberClassComponent.js

+        '%s: It is not recommended to assign props directly to state ' +
+          "because updates to props won't be reflected in state. " +
+          'In most cases, it is better to use props directly.',
+        getComponentName(workInProgress) || 'A component',


nit: I think we generally use "Component" as the default name in warnings when it's followed by ":" (e.g. "Component: blah". We use "A component" when it's part of a sentence (e.g. "A component blah")

bvaughn

Left a nit but we could do this post-merge.

LGTM otherwise.

gaearon

This warning should be deduplicated by component name. Otherwise it'll spam the console for any component that's used in a large number of places.

bvaughn · 2018-04-03T18:05:48Z

Nice catch 😄

veekas · 2018-04-07T19:42:43Z

Latest commit should address @gaearon's request!

bvaughn · 2018-04-09T17:29:23Z

packages/react-reconciler/src/ReactFiberClassComponent.js

+      if (!didWarnAboutDirectlyAssigningPropsToState.has(componentName)) {
+        didWarnAboutDirectlyAssigningPropsToState.add(componentName);
+        warning(
+          instance.state !== instance.props,


I think we should move this check up a level:

if (__DEV__) { if (instance.state === instance.props) { const componentName = getComponentName(workInProgress) || 'Component'; if (!didWarnAboutDirectlyAssigningPropsToState.has(componentName)) { didWarnAboutDirectlyAssigningPropsToState.add(componentName); warning(false, ...);

This way we don't add things to the Set unnecessarily, and we also don't potentially suppress a later warning (e.g. if we encounter 2 components with the same name- and the second one assigns props to state but the first one doesn't).

Ah, yes, of course. Thanks for catching that. (Edit: fixed)

bvaughn

👍 Looks good to me.

veekas · 2018-04-16T21:28:40Z

Hey @gaearon or @bvaughn, anything else I can do to help get this merged?

pull-bot · 2018-04-25T16:59:42Z

Details of bundled changes.

Comparing: 29287f0...4268592

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	649.61 KB	650.21 KB	152.61 KB	152.73 KB	UMD_DEV
react-dom.development.js	+0.1%	+0.1%	645.75 KB	646.34 KB	151.5 KB	151.62 KB	NODE_DEV
ReactDOM-dev.js	+0.1%	+0.1%	652.79 KB	653.44 KB	149.5 KB	149.62 KB	FB_WWW_DEV

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.1%	+0.1%	441.74 KB	442.33 KB	99.92 KB	100.03 KB	UMD_DEV
react-art.development.js	+0.2%	+0.1%	374.28 KB	374.87 KB	82.82 KB	82.94 KB	NODE_DEV
ReactART-dev.js	+0.2%	+0.1%	364.38 KB	365.04 KB	77.54 KB	77.66 KB	FB_WWW_DEV

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.2%	+0.1%	371.11 KB	371.7 KB	81.21 KB	81.33 KB	UMD_DEV
react-test-renderer.development.js	+0.2%	+0.1%	367.24 KB	367.83 KB	80.26 KB	80.37 KB	NODE_DEV
ReactTestRenderer-dev.js	+0.2%	+0.1%	372.04 KB	372.69 KB	79.21 KB	79.33 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.2%	+0.2%	355.33 KB	355.93 KB	76.89 KB	77 KB	NODE_DEV
react-reconciler-persistent.development.js	+0.2%	+0.2%	353.9 KB	354.49 KB	76.31 KB	76.42 KB	NODE_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.1%	486.01 KB	486.67 KB	107.52 KB	107.63 KB	RN_FB_DEV
ReactNativeRenderer-dev.js	+0.1%	+0.1%	485.75 KB	486.4 KB	107.46 KB	107.57 KB	RN_OSS_DEV
ReactFabric-dev.js	+0.1%	+0.1%	476.2 KB	476.85 KB	105.09 KB	105.2 KB	RN_FB_DEV
ReactFabric-dev.js	+0.1%	+0.1%	476.23 KB	476.89 KB	105.11 KB	105.22 KB	RN_OSS_DEV

Generated by 🚫 dangerJS

veekas · 2018-04-27T05:55:59Z

Hey @gaearon, just want to ping you again about this PR

veekas · 2018-08-28T03:41:55Z

Hi @gaearon, I've rebased/squashed my commits to fix the merge conflict that happened over the past few months. I'll give this opportunity to one of the volunteers in #11593 if any further changes are requested. Thanks!

gaearon · 2018-08-28T13:17:45Z

Thanks!

stnwk · 2018-09-08T20:54:04Z

Is there any reason this is being outputted as an error on the console instead of a warning? @veekas @gaearon

gaearon · 2018-09-08T22:33:59Z

@stnwk We output all warnings through console.error because React warnings generally point out legitimate bugs in your code. If you're 100% confident you know why you're copying props to state like this, you can change it to this.state = {...this.props} to be explicit. But it's usually a mistake.

stnwk · 2018-09-09T09:50:01Z

Okay, thanks for the explanation :)

…facebook#11658) * add test to warn if setting this.state to this.props referentially * Avoid an extra check

facebook-github-bot added the CLA Signed label Nov 25, 2017

aweary reviewed Nov 28, 2017

View reviewed changes

gaearon reviewed Jan 5, 2018

View reviewed changes

raunofreiberg reviewed Mar 16, 2018

View reviewed changes

bvaughn reviewed Apr 3, 2018

View reviewed changes

bvaughn approved these changes Apr 3, 2018

View reviewed changes

gaearon requested changes Apr 3, 2018

View reviewed changes

bvaughn reviewed Apr 9, 2018

View reviewed changes

bvaughn approved these changes Apr 9, 2018

View reviewed changes

add test to warn if setting this.state to this.props referentially

cd0a0dd

veekas force-pushed the master branch from 877e6e9 to cd0a0dd Compare August 28, 2018 02:53

veekas mentioned this pull request Aug 28, 2018

Warn if this.state is set to this.props referentially #11593

Closed

Avoid an extra check

4268592

gaearon approved these changes Aug 28, 2018

View reviewed changes

gaearon merged commit 672e859 into facebook:master Aug 28, 2018

gaearon mentioned this pull request Sep 5, 2018

Changelog for 16.5.0 #13571

Merged

asapach mentioned this pull request Sep 6, 2018

React 16.5.0: It is not recommended to assign props directly to state mobxjs/mobx-react#545

Closed

This was referenced Sep 10, 2018

Update react-dom in / from 16.4.1 to 16.5.0 severest/retrobot#229

Closed

Update react in / from 16.4.2 to 16.5.0 severest/retrobot#230

Merged

yched mentioned this pull request Sep 11, 2018

Warning with react 16.5.0 : "It is not recommended to assign props directly to state..." podefr/react-debounce-render#11

Closed

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019

Add warning to prevent setting this.state to this.props referentially (…

803e58c

…facebook#11658) * add test to warn if setting this.state to this.props referentially * Avoid an extra check

Add warning to prevent setting this.state to this.props referentially #11658

Add warning to prevent setting this.state to this.props referentially #11658

Conversation

veekas commented Nov 25, 2017

gaearon commented Nov 25, 2017 • edited

veekas commented Nov 25, 2017

Choose a reason for hiding this comment

veekas Nov 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veekas commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veekas commented Mar 16, 2018 • edited

raunofreiberg Mar 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Mar 17, 2018 • edited

Choose a reason for hiding this comment

veekas commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn left a comment

Choose a reason for hiding this comment

gaearon left a comment

Choose a reason for hiding this comment

bvaughn commented Apr 3, 2018

veekas commented Apr 7, 2018 • edited

bvaughn Apr 9, 2018 • edited

Choose a reason for hiding this comment

veekas Apr 9, 2018 • edited

Choose a reason for hiding this comment

bvaughn left a comment

Choose a reason for hiding this comment

veekas commented Apr 16, 2018

pull-bot commented Apr 25, 2018 • edited

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

veekas commented Apr 27, 2018

veekas commented Aug 28, 2018

gaearon commented Aug 28, 2018

stnwk commented Sep 8, 2018

gaearon commented Sep 8, 2018

stnwk commented Sep 9, 2018

gaearon commented Nov 25, 2017 •

edited

veekas Nov 28, 2017 •

edited

veekas commented Mar 16, 2018 •

edited

raunofreiberg Mar 16, 2018 •

edited

bvaughn Mar 17, 2018 •

edited

veekas commented Apr 7, 2018 •

edited

bvaughn Apr 9, 2018 •

edited

veekas Apr 9, 2018 •

edited

pull-bot commented Apr 25, 2018 •

edited