[compiler] Make ref enforcement on by default #30716
Merged
Conversation
This file contains 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
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. [ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This was referenced Aug 15, 2024
mvitousek
added a commit
that referenced
this pull request
Aug 15, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. ghstack-source-id: 341048b4971630bbbda5dc00157fb86826970713 Pull Request resolved: #30716
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. [ghstack-poisoned]
mvitousek
added a commit
that referenced
this pull request
Aug 15, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. ghstack-source-id: bb58c509352bb4a392a87188b10894fb1cf86eca Pull Request resolved: #30716
| expect(defaultConfig.validateNoSetStateInRender).toBe(true); | ||
|
|
||
| const config = parseConfigPragma( | ||
| '@enableUseTypeAnnotations @validateRefAccessDuringRender:true @validateNoSetStateInRender:false', | ||
| '@enableUseTypeAnnotations @validateRefAccessDuringRender:false @validateNoSetStateInRender:false', |
There was a problem hiding this comment.
nit: the idea of this test was to test both the @flag, @flag:true, and @flag:false cases, so maybe choose a different off-by-default flag to use instead
Comment on lines
11
to
15
| return useCallback(() => { | ||
| if (ref != null) { | ||
| ref.current(); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
hmm this actually seems like it should be allowed: this could be returning a callback that is only called in an event/effect
josephsavona
requested changes
Aug 15, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. [ghstack-poisoned]
This was referenced Aug 16, 2024
Good call, addressed in 30724 |
mvitousek
added a commit
that referenced
this pull request
Aug 16, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. ghstack-source-id: 4dbf7050f5e324027201fc7efee0561554b3eda1 Pull Request resolved: #30716
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. [ghstack-poisoned]
mvitousek
added a commit
that referenced
this pull request
Aug 16, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. ghstack-source-id: e3d85445f7cd95297832dd4d836edc160fe9c2ef Pull Request resolved: #30716
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. [ghstack-poisoned]
mvitousek
added a commit
that referenced
this pull request
Aug 16, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. ghstack-source-id: d3ab5f1b28b7aed0f0d6d69547bb638a1e326b66 Pull Request resolved: #30716
mvitousek
added a commit
that referenced
this pull request
Aug 16, 2024
Summary: The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default. ghstack-source-id: d3ab5f1b28b7aed0f0d6d69547bb638a1e326b66 Pull Request resolved: #30716
4 tasks
4 tasks
|
This change has on the whole exposed lots of potential bugs for us - thanks. The main case where we see alot of non bugs is passing the ref into a custom hook or react hook (useImperitiveHandle). In these cases it seems it usually uses useLayoutEffect and then reads the ref inside useLayoutEffect. should it be relaxed to allow passing refs to hooks? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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):
Summary:
The change earlier in this stack makes it less safe to have ref enforcement disabled. This diff enables it by default.