Warn on direct state manipulation (#2272)

[iansu]

Adds a warning when setting state directly using this.state instead of this.setState().

Resolves #2272

[facebook-github-bot]

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[aweary]

I'm not sure how useful this warning would really be, it doesn't give you any information about what was mutated or where it happened. I know that providing those details would make this more difficult, but I don't think it provides a whole lot of benefit for the cost as it stands.

[iansu]

That's fair. Adding the name of the component to the warning is relatively easy. I should have done that in the first place. Showing what was changed is definitely more difficult. If people think that's valuable then I'm willing to look into it.

[oksas]

@iansu were you still working on this?

[iansu]

@oksas I'd like to continue working on this. I have some ideas to improve the implementation, it's just unclear if those improvements will be enough to justify including this feature. Also, for some reason I didn't get a notification that @gaearon mentioned this pull request. 😞

Possible improvements:

• Only create a copy of the component state in dev so there's no performance impact in production
• Add the name of the component to the warning message
• Show what part of the state was changed and where (I'm not exactly sure how to implement this one, so any feedback would be appreciated)

[aweary]

@iansu can you resolve the merge conflicts? This LGTM, I think we can move forward with it once its mergeable.

[iansu]

Done. Just waiting for the tests to run on CI now.

[iansu]

@aweary this is done now. Not sure if you saw my previous message because I didn't explicitly mention you.

[aweary]

@iansu thanks! Can you run the ./scripts/fiber/record-tests script and commit the results please?

[iansu]

@aweary done! Looks like a couple of tests might be failing because of my changes. I'm not too familiar with Fiber though. Let me know if I should try and fix them.

[aweary]

@iansu sorry I'm actually not too familiar with fiber yet either, and I'm not sure if we want to implement this in fiber as well before merging. cc @gaearon for those questions

[iansu]

@aweary @gaearon sounds good. Is there any documentation on running the Fiber tests? The "How to Contribute" page just says to run npm test. I was a bit surprised when this first ran on CI and it ran three different sets of tests. I ended up looking at the console output there to figure out how to run the SSR tests but even that seems to have changed with the move to CircleCI. Might be a good opportunity for someone to contribute to the docs.

[bvaughn]

Is there any documentation on running the Fiber tests?

@iansu: Sorry for chiming in late, but you can run tests against Fiber using the REACT_DOM_JEST_USE_FIBER env flag (eg REACT_DOM_JEST_USE_FIBER=true jest). Keep in mind that some failures are expected currently. The ./scripts/fiber/record-tests will let you know if any new failures were introduced because of local changes though.

That said, I think we actually want to pass on this change. Even though we warn if this.state is replaced, we still support it (and will likely continue to support it). I don't think it's worth the additional cost of cloning and doing a shallow comparison of state after each lifecycle method just to warn about something that we actually support (even though it's not recommended).

So I'm going to close this PR. Sorry for the inconvenience! And thanks a ton for the time and effort you spent on it. 🙇

[iansu]

@bvaughn Well that's disappointing but I appreciate the explanation. I'll keep looking for other opportunities to contribute. 😀