compiler: only resolve globals and react imports #29190
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
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. [ghstack-poisoned]
This was referenced May 21, 2024
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
josephsavona
added a commit
that referenced
this pull request
May 21, 2024
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. ghstack-source-id: 3494c99d0f61570b9a67c3151e01f4204101dacd Pull Request resolved: #29190
facebook-github-bot
added
CLA Signed
React Core Team
josephsavona
commented
May 21, 2024
| const $ = _c(5); | ||
| const [state, setState] = useState("hello"); | ||
| let t0; | ||
| if ($[0] !== setState) { |
There was a problem hiding this comment.
yay! we know this is not React.setState so we treat the setter as reactive. now i need to actually make this value reactive and test that it works via snap.
|
Comparing: bf046e8...1d6986b 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
|
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
May 21, 2024
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. ghstack-source-id: 0c26e76d41512db0976de16bd33475723d6b99f6 Pull Request resolved: #29190
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
May 21, 2024
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. ghstack-source-id: 0f14bb17b970fb99ad5e4830fde98c3abbfab74a Pull Request resolved: #29190
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
May 21, 2024
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. Still working on more tests. ghstack-source-id: a54ae2a7171fb42a5aad5b6537d6016e66778e4a Pull Request resolved: #29190
josephsavona
commented
May 21, 2024
| @@ -56,71 +56,78 @@ function useFragment(_arg1, _arg2) { | |||
| } | |||
|
|
|||
| function Component(props) { | |||
There was a problem hiding this comment.
this one changes because useFragment is defined locally, so we don't pickup the global declaration of useFragment that we inject for tests (which comes from shared-runtime)
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this. [ghstack-poisoned]
josephsavona
added a commit
that referenced
this pull request
May 24, 2024
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this. ghstack-source-id: 5e9a8f6e0d23659c72a9d041e8d394b83d6e526d Pull Request resolved: #29190
josephsavona
commented
May 24, 2024
Comment on lines
+37
to
+38
| if ($[0] !== state) { | ||
| t0 = [state, setState]; |
There was a problem hiding this comment.
we know that this is the real useState, so setState is non-reactive here
josephsavona
commented
May 24, 2024
Comment on lines
+39
to
+41
| if ($[0] === Symbol.for("react.memo_cache_sentinel")) { | ||
| t0 = () => { | ||
| setState((s) => s + 1); |
There was a problem hiding this comment.
even though it's aliased on import, we know that useReactState is actually useState, so setState is non-reactive
josephsavona
requested review from
gsathya and
mofeiZ
and removed request for
gsathya
mofeiZ
approved these changes
May 24, 2024
josephsavona
added a commit
that referenced
this pull request
May 24, 2024
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local `Array` reference won't get confused with our Array global declaration, a local `useState` (or import from something other than React) won't get confused as `React.useState()`, etc. I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this. ghstack-source-id: 5e9a8f6e0d23659c72a9d041e8d394b83d6e526d Pull Request resolved: #29190
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):
Updates Environment#getGlobalDeclaration() to only resolve "globals" if they are a true global or an import from react/react-dom. We still keep the logic to resolve hook-like names as custom hooks. Notably, this means that a local
Arrayreference won't get confused with our Array global declaration, a localuseState(or import from something other than React) won't get confused asReact.useState(), etc.I tried to write a proper fixture test to test that we react to changes to a custom setState setter function, but I think there may be an issue with snap and how it handles re-renders from effects. I think the tests are good here but open to feedback if we want to go down the rabbit hole of figuring out a proper snap test for this.