React: Warn if this.state is set to this.props referentially

[gaearon]

See this example: https://github.com/facebook/react/issues/11508#issuecomment-345464797.

I think it probably reflects a misunderstanding of how props and state work, and we should detect and warn if we see this.

[sw-yx]

Happy to take!

edit: instead of doing this myself i am working with @veekas to get him started contributing to react, below

[gaearon]

Sounds good.

[veekas]

I'm a new contributor, but working with @sw-yx on this.

I'm currently trying to figure out how to catch if the user is trying to set this.state = props. Do you have any tips?

I currently am looking at putting the warning in ReactFiberClassComponent.js. Something like the following:

// the following is in the checkClassInstance function
      const noSetStatesFromProps = !(instance.state === parent.props); // this logic is wrong, but what is correct?
      warning(
        noSetStatesFromProps,
        'this.state should not be set to this.props referentially. When ' +
          'implementing the constructor for a React.Component subclass, you ' +
          'should call super(props) before any other statement. To initialize ' +
          'state locally, just assign an object to this.state in the constructor.'
      );

Here are the changes I've made so far, for reference.

Thanks for your help!

[sw-yx]

in discussing with veekas, i wasn't sure how to do this one either. there are two questions here.

1. how to test if this.state is being set to React.Component.props
2. -where- to test it

my best guess is to stick the test inside of the componentWillMount lifecycle method execution as that is the earliest place we can check this.state.

[gaearon]

Can't you check instance.state === instance.props? I don't understand what parent is or how it is relevant.

my best guess is to stick the test inside of the componentWillMount lifecycle method execution as that is the earliest place we can check this.state

Not sure what you mean. The original direction by @veekas makes sense to me. Test it in React itself (not some component's methods) right after the instance is created. In fact you can find where React calls the constructor in this file, and check right after it.

[isaacchien]

Hey @veekas, are you still working on this? I'd like to try finishing it.

[sw-yx]

He's almost done with it afaik. look at his PR

[diegoborda]

Hi! I see this issue is taken but it seems still. @sw-yx @veekas Are you taking it? I can finish it if you want.

[veekas]

Oh wow, completely forgot about this. Thanks for the ping @diegoborda. I'll have it done asap.

[benbakhar]

Hey guys, is this still in progress? would be happy to take it.

[veekas]

@benbakhar If you'd like, please take a look at #11658 and review it if you see room for improvement! Same goes for @diegoborda, @isaacchien, et al. If any further changes are requested by @gaearon or other contributors, I'd love to pass this baton to one of you.

[gaearon]

Sorry we dropped this. The past months have been pretty intense. :-)

[gaearon]

Fixed in https://github.com/facebook/react/pull/11658

[veekas]

Thanks! No worries, glad to officially have done a little bit to make React better. =)