compiler: Allow global mutation in jsx props #29591
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
[ghstack-poisoned]
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
josephsavona
added a commit
that referenced
this pull request
May 25, 2024
ghstack-source-id: 12c5a86ebe1487fee57d5cc272920dafda23cc22 Pull Request resolved: #29591
facebook-github-bot
added
the
React Core Team
|
Comparing: b078c81...a15a43e Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
Fixes https://x.com/raibima/status/1794395807216738792 The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect. Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways: * For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically. * Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
May 25, 2024
Fixes https://x.com/raibima/status/1794395807216738792 The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect. Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways: * For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically. * Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render. ghstack-source-id: 4aba4e6d214fd6c062e4029294efe9b8fe25cd83 Pull Request resolved: #29591
|
Fixes #29106 |
This was referenced May 25, 2024
gsathya
approved these changes
May 29, 2024
josephsavona
added a commit
that referenced
this pull request
May 29, 2024
Fixes https://x.com/raibima/status/1794395807216738792 The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect. Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways: * For all functions that modify globals, codegen an assertion or warning into the function that fires if it's called "during render". We'd have to precisely define what "during render" is, but this would at least help developers catch this dynamically. * Use the type system to distinguish "event/effect" and "render" functions to help developers avoid accidentally mutating globals during render. ghstack-source-id: 4aba4e6d214fd6c062e4029294efe9b8fe25cd83 Pull Request resolved: #29591
1 task
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):
Fixes #29106 and https://x.com/raibima/status/1794395807216738792
The issue is that if you pass a global-modifying function as prop to JSX, we currently report that it's invalid to modify a global during rendering. The problem is that we don't really know when/if the child component will actually call that function prop. It would be against the rules to call the function during render, but it's totally fine to call it during an event handler or from a useEffect.
Since we don't know at the call-site how the child will use the function, we should allow such calls. In the future we could improve this in a few ways: