[compiler] Off-by-default validation against setState directly in passive effect #30685
Conversation
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
…sive effect Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
josephsavona
added a commit
that referenced
this pull request
Aug 14, 2024
…sive effect Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. ghstack-source-id: da72a35 Pull Request resolved: #30685
facebook-github-bot
added
CLA Signed
React Core Team
…ctly in passive effect" Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. Rationale: https://react.dev/learn/you-might-not-need-an-effect [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
Aug 14, 2024
…sive effect Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. ghstack-source-id: 5f385dd Pull Request resolved: #30685
mvitousek
reviewed
Aug 14, 2024
| * alternatives. See https://react.dev/learn/you-might-not-need-an-effect for examples. | ||
| */ | ||
| export function validateNoSetStateInPassiveEffects(fn: HIRFunction): void { | ||
| const setStateFunctions: Map<IdentifierId, Place> = new Map(); |
There was a problem hiding this comment.
This is profoundly nitpicky given how hard it is to construct an example of this, but we might want to first detect all the setStateFunctions in a component before taking another pass and detecting errors, because of setState functions that might be hoisted. Compare against this, which has the definitions reordered to not be hoisted
There was a problem hiding this comment.
Per offline discussion the same issue applies to several other validations, let's land this and do a follow-up pass to apply the same two-phase approach to all the validations (and maybe take that as a chance to create more of a framework for validations)
josephsavona
added a commit
that referenced
this pull request
Aug 15, 2024
…sive effect Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to _schedule_ setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render. This validation is off by default, i'm putting this up for discussion and to experiment with it internally. ghstack-source-id: 5f385dd Pull Request resolved: #30685
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Stack from ghstack (oldest at bottom):
Per discussion today, adds validation against calling setState "during" passive effects. Basically, it's fine to schedule setState to be called (via a timeout, listener, etc) but generally not recommended to call setState during the effect since that will trigger a cascading render.
This validation is off by default, i'm putting this up for discussion and to experiment with it internally.
Rationale: https://react.dev/learn/you-might-not-need-an-effect