-
Notifications
You must be signed in to change notification settings - Fork 47.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fresh] Fix edge case with early function call #17824
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e47cd31:
|
Details of bundled changes.Comparing: 64aae7b...e47cd31 react-refresh
Size changes (stable) |
Details of bundled changes.Comparing: 64aae7b...e47cd31 react-refresh
Size changes (experimental) |
@@ -601,9 +601,14 @@ export function _getMountedRootCount() { | |||
// 'useState{[foo, setFoo]}(0)', | |||
// () => [useCustomHook], /* Lazy to avoid triggering inline requires */ | |||
// ); | |||
type SignatureStatus = 'needsSignature' | 'needsCustomHooks' | 'resolved'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice readability change 👍
Hi, I'm having this issue in Expo which seems to be using version 0.4.2. I'm using patch-package for now to add this line:
Would you mind to release this fix as 0.4.3? Based on my yarn.lock file, the other dependencies always use |
What is the actual code triggering this in your case? |
I released this PR as 0.4.3. |
Awesome! It works 😃 actual code: const App = () => <Text>{useUser()}</Text>
// imported
const [useUser, setUser] = createGlobalHook<string>('')
export { useUser, setUser }
fetchUser().then(setUser)
// imported
export function createGlobalHook(state) {
let effects: any[] = []
return [useGlobal, setGlobal, get]
function useGlobal() {
const [, effect] = useState(state)
useEffect(() => {
effects = effects.concat(effect)
return () => effects = effects.filter(eff => eff !== effect)
}, [])
return state
}
function setGlobal(newState) {
if (typeof newState === 'function') {
state = newState(state)
} else {
state = newState
}
effects.forEach(s => s(state))
return state
}
function get() {
return state
}
} |
When a component calls custom Hooks, we "collect" a signature in two steps.
react/packages/react-refresh/src/ReactFreshRuntime.js
Lines 582 to 603 in 64aae7b
For example, given this component:
we want to:
App
has twouseFancyState
calls._s(App, 'useFancyState(x) useFancyState(y)', () => [useFancyState, useFancyState])
App
for the first time, trigger the function to save their identities_s()
We intentionally trigger the function to save function identities lazily on first render. This is so that we don't change the module definition order when you have inline requires.
We reuse the same function to minimize the transform noise. So we count calls. First call saves
() => [useFancyState, useFancyState]
, second call actually triggers it to get references. Next calls don't do anything.This usually works, but it breaks in the corner case like this:
This is because we end up inside
App
before the first call injected by our transform runs. So we get the "inside" call before we get the "outside" call.This is edge casey but can happen in environments like CodeSandbox where you write all code in one file.
The fix is to not rely on the call index, and instead to look at what we're passing. With this fix, we ignore all empty
_s()
calls until after we actually get the first_s(App, ...)
registration call. To make this clearer, I'm changing a number tracking the call number to a status enum.A regression test is added. This should fix this broken case on CodeSandbox. (After CodeSandbox updates
react-refresh
to the version with this fix.)