Forbid readContext() during render-phase class setState() reducer form#14670
Closed
gaearon wants to merge 2 commits intofacebook:masterfrom
Closed
Forbid readContext() during render-phase class setState() reducer form#14670gaearon wants to merge 2 commits intofacebook:masterfrom
gaearon wants to merge 2 commits intofacebook:masterfrom
Conversation
gaearon
commented
Jan 23, 2019
| expect(ReactNoop.flush).toThrow( | ||
| 'Context can only be read while React is rendering', | ||
| ), | ||
| ).toWarnDev('Cannot update during an existing state transition', { |
Collaborator
Author
There was a problem hiding this comment.
We warn anyway so maybe it isn't worth it?
Collaborator
|
The reason to do this is because the semantics are currently undefined — what happens when context changes? Re-rendering will have no effect because the update already happened — and we want to preserve option value to possibly support it properly in the future. Like for code splitting reducers. Same reason we warn about reading context inside Hook reducer.
|
|
ReactDOM: size: 0.0%, gzip: 🔺+0.2% Details of bundled changes.Comparing: f0befae...d0fabf1 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
Collaborator
Author
|
That's true but we already unconditionally warn for render phase I guess we probably mute that warning at FB though? |
Contributor
|
Added a comment that I don’t think we should do this in prod. |
Collaborator
|
Yeah warning is sufficient |
Collaborator
Author
|
Do we need an extra warning for this specifically? This test already warns with "Cannot update during an existing state transition". |
gaearon
force-pushed
the
forbid-ren-phase-setstate-cls
branch
from
b555c33 to
d0fabf1
Compare
Collaborator
Author
|
I made this depend on #14672 and added a similar DEV-only warning on top. |
Collaborator
Author
|
It's annoying to have so many small ones. I'll send a combined one. |
Collaborator
Author
|
Superseded by #14677 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #14672
Addresses #14653 (comment) (I think?)
Not sure we want to actually do it but figured I'd send a PR.