Skip to content

Commit 255d9ac

Browse files
authored
[Fresh] Fix edge case with early function call (#17824)
1 parent 64aae7b commit 255d9ac

File tree

2 files changed

+101
-7
lines changed

2 files changed

+101
-7
lines changed

packages/react-refresh/src/ReactFreshRuntime.js

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -601,9 +601,14 @@ export function _getMountedRootCount() {
601601
// 'useState{[foo, setFoo]}(0)',
602602
// () => [useCustomHook], /* Lazy to avoid triggering inline requires */
603603
// );
604+
type SignatureStatus = 'needsSignature' | 'needsCustomHooks' | 'resolved';
604605
export function createSignatureFunctionForTransform() {
605606
if (__DEV__) {
606-
let call = 0;
607+
// We'll fill in the signature in two steps.
608+
// First, we'll know the signature itself. This happens outside the component.
609+
// Then, we'll know the references to custom Hooks. This happens inside the component.
610+
// After that, the returned function will be a fast path no-op.
611+
let status: SignatureStatus = 'needsSignature';
607612
let savedType;
608613
let hasCustomHooks;
609614
return function<T>(
@@ -612,16 +617,25 @@ export function createSignatureFunctionForTransform() {
612617
forceReset?: boolean,
613618
getCustomHooks?: () => Array<Function>,
614619
): T {
615-
switch (call++) {
616-
case 0:
617-
savedType = type;
618-
hasCustomHooks = typeof getCustomHooks === 'function';
619-
setSignature(type, key, forceReset, getCustomHooks);
620+
switch (status) {
621+
case 'needsSignature':
622+
if (type !== undefined) {
623+
// If we received an argument, this is the initial registration call.
624+
savedType = type;
625+
hasCustomHooks = typeof getCustomHooks === 'function';
626+
setSignature(type, key, forceReset, getCustomHooks);
627+
// The next call we expect is from inside a function, to fill in the custom Hooks.
628+
status = 'needsCustomHooks';
629+
}
620630
break;
621-
case 1:
631+
case 'needsCustomHooks':
622632
if (hasCustomHooks) {
623633
collectCustomHooksForSignature(savedType);
624634
}
635+
status = 'resolved';
636+
break;
637+
case 'resolved':
638+
// Do nothing. Fast path for all future renders.
625639
break;
626640
}
627641
return type;

packages/react-refresh/src/__tests__/ReactFreshIntegration-test.js

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,86 @@ describe('ReactFreshIntegration', () => {
605605
}
606606
});
607607

608+
it('does not get confused when component is called early', () => {
609+
if (__DEV__) {
610+
render(`
611+
// This isn't really a valid pattern but it's close enough
612+
// to simulate what happens when you call ReactDOM.render
613+
// in the same file. We want to ensure this doesn't confuse
614+
// the runtime.
615+
App();
616+
617+
function App() {
618+
const [x, setX] = useFancyState('X');
619+
const [y, setY] = useFancyState('Y');
620+
return <h1>A{x}{y}</h1>;
621+
};
622+
623+
function useFancyState(initialState) {
624+
// No real Hook calls to avoid triggering invalid call invariant.
625+
// We only want to verify that we can still call this function early.
626+
return initialState;
627+
}
628+
629+
export default App;
630+
`);
631+
let el = container.firstChild;
632+
expect(el.textContent).toBe('AXY');
633+
634+
patch(`
635+
// This isn't really a valid pattern but it's close enough
636+
// to simulate what happens when you call ReactDOM.render
637+
// in the same file. We want to ensure this doesn't confuse
638+
// the runtime.
639+
App();
640+
641+
function App() {
642+
const [x, setX] = useFancyState('X');
643+
const [y, setY] = useFancyState('Y');
644+
return <h1>B{x}{y}</h1>;
645+
};
646+
647+
function useFancyState(initialState) {
648+
// No real Hook calls to avoid triggering invalid call invariant.
649+
// We only want to verify that we can still call this function early.
650+
return initialState;
651+
}
652+
653+
export default App;
654+
`);
655+
// Same state variables, so no remount.
656+
expect(container.firstChild).toBe(el);
657+
expect(el.textContent).toBe('BXY');
658+
659+
patch(`
660+
// This isn't really a valid pattern but it's close enough
661+
// to simulate what happens when you call ReactDOM.render
662+
// in the same file. We want to ensure this doesn't confuse
663+
// the runtime.
664+
App();
665+
666+
function App() {
667+
const [y, setY] = useFancyState('Y');
668+
const [x, setX] = useFancyState('X');
669+
return <h1>B{x}{y}</h1>;
670+
};
671+
672+
function useFancyState(initialState) {
673+
// No real Hook calls to avoid triggering invalid call invariant.
674+
// We only want to verify that we can still call this function early.
675+
return initialState;
676+
}
677+
678+
export default App;
679+
`);
680+
// Hooks were re-ordered. This causes a remount.
681+
// Therefore, Hook calls don't accidentally share state.
682+
expect(container.firstChild).not.toBe(el);
683+
el = container.firstChild;
684+
expect(el.textContent).toBe('BXY');
685+
}
686+
});
687+
608688
it('does not get confused by Hooks defined inline', () => {
609689
// This is not a recommended pattern but at least it shouldn't break.
610690
if (__DEV__) {

0 commit comments

Comments
 (0)