Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Aug 28, 2018

Conversation

veekas
Copy link

@veekas veekas commented Nov 25, 2017

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
Copy link
Collaborator

gaearon commented Nov 25, 2017

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
Copy link
Author

veekas commented Nov 25, 2017

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

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

const noSetStatesFromProps = !(instance.state === instance.props);
warning(
noSetStatesFromProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
  `...`
)

Copy link
Author

@veekas veekas Nov 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

veekas commented Jan 3, 2018

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?

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; ' +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@veekas
Copy link
Author

veekas commented Mar 16, 2018

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!

'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',
Copy link
Contributor

@raunofreiberg raunofreiberg Mar 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bvaughn bvaughn Mar 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

veekas commented Apr 2, 2018

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

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." +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a trailing space (" ") here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

'%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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LGTM otherwise.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

bvaughn commented Apr 3, 2018

Nice catch 😄

@veekas
Copy link
Author

veekas commented Apr 7, 2018

Latest commit should address @gaearon's request!

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

@bvaughn bvaughn Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

@veekas veekas Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me.

@veekas
Copy link
Author

veekas commented Apr 16, 2018

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

@pull-bot
Copy link

pull-bot commented Apr 25, 2018

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
Copy link
Author

veekas commented Apr 27, 2018

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

@veekas
Copy link
Author

veekas commented Aug 28, 2018

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 gaearon merged commit 672e859 into facebook:master Aug 28, 2018
@gaearon
Copy link
Collaborator

gaearon commented Aug 28, 2018

Thanks!

@stnwk
Copy link

stnwk commented Sep 8, 2018

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

@gaearon
Copy link
Collaborator

gaearon commented Sep 8, 2018

@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
Copy link

stnwk commented Sep 9, 2018

Okay, thanks for the explanation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants