From 4387d752dadada921938f9ea171d065b44a769c1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 2 Nov 2022 22:50:45 -0400 Subject: [PATCH] Allow more hooks to be added when replaying mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, if you call setState in render, you must render the exact same hooks as during the first render pass. I'm about to add a behavior where if something suspends, we can reuse the hooks from the previous attempt. That means during initial render, if something suspends, we should be able to reuse the hooks that were already created and continue adding more after that. This will error in the current implementation because of the expectation that every render produces the same list of hooks. In this commit, I've changed the logic to allow more hooks to be added when replaying. But only during a mount — if there's already a current fiber, then the logic is unchanged, because we shouldn't add any additional hooks that aren't in the current fiber's list. Mounts are special because there's no current fiber to compare to. I haven't change any other behavior yet. The reason I've put this into its own step is there are a couple tests that intentionally break the Hook rule, to assert that React errors in these cases, and those happen to be coupled to the behavior. This is undefined behavior that is always accompanied by a warning and/or error. So the change should be safe. --- .../ReactDOMServerIntegrationHooks-test.js | 46 +++++++++++-------- .../src/ReactFiberHooks.new.js | 19 +++++++- .../src/ReactFiberHooks.old.js | 19 +++++++- .../src/__tests__/ReactHooks-test.internal.js | 4 +- 4 files changed, 65 insertions(+), 23 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index 9e59cee39a..48ab3565c6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -430,26 +430,6 @@ describe('ReactDOMServerHooks', () => { expect(domNode.textContent).toEqual('hi'); }); - itThrowsWhenRendering( - 'with a warning for useRef inside useReducer', - async render => { - function App() { - const [value, dispatch] = useReducer((state, action) => { - useRef(0); - return state + 1; - }, 0); - if (value === 0) { - dispatch(); - } - return value; - } - - const domNode = await render(, 1); - expect(domNode.textContent).toEqual('1'); - }, - 'Rendered more hooks than during the previous render', - ); - itRenders('with a warning for useRef inside useState', async render => { function App() { const [value] = useState(() => { @@ -686,6 +666,32 @@ describe('ReactDOMServerHooks', () => { ); }); + describe('invalid hooks', () => { + it('warns when calling useRef inside useReducer', async () => { + function App() { + const [value, dispatch] = useReducer((state, action) => { + useRef(0); + return state + 1; + }, 0); + if (value === 0) { + dispatch(); + } + return value; + } + + let error; + try { + await serverRender(); + } catch (x) { + error = x; + } + expect(error).not.toBe(undefined); + expect(error.message).toContain( + 'Rendered more hooks than during the previous render', + ); + }); + }); + itRenders( 'can use the same context multiple times in the same function', async render => { diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index b076675272..09828fd12b 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -839,7 +839,24 @@ function updateWorkInProgressHook(): Hook { // Clone from the current hook. if (nextCurrentHook === null) { - throw new Error('Rendered more hooks than during the previous render.'); + const currentFiber = currentlyRenderingFiber.alternate; + if (currentFiber === null) { + // This is the initial render. This branch is reached when the component + // suspends, resumes, then renders an additional hook. + const newHook: Hook = { + memoizedState: null, + + baseState: null, + baseQueue: null, + queue: null, + + next: null, + }; + nextCurrentHook = newHook; + } else { + // This is an update. We should always have a current hook. + throw new Error('Rendered more hooks than during the previous render.'); + } } currentHook = nextCurrentHook; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 73479626e1..08f3ee46ff 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -839,7 +839,24 @@ function updateWorkInProgressHook(): Hook { // Clone from the current hook. if (nextCurrentHook === null) { - throw new Error('Rendered more hooks than during the previous render.'); + const currentFiber = currentlyRenderingFiber.alternate; + if (currentFiber === null) { + // This is the initial render. This branch is reached when the component + // suspends, resumes, then renders an additional hook. + const newHook: Hook = { + memoizedState: null, + + baseState: null, + baseQueue: null, + queue: null, + + next: null, + }; + nextCurrentHook = newHook; + } else { + // This is an update. We should always have a current hook. + throw new Error('Rendered more hooks than during the previous render.'); + } } currentHook = nextCurrentHook; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 37fd06f1e0..63dc3c04d0 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1071,7 +1071,9 @@ describe('ReactHooks', () => { expect(() => { expect(() => { ReactTestRenderer.create(); - }).toThrow('Rendered more hooks than during the previous render.'); + }).toThrow( + 'Should have a queue. This is likely a bug in React. Please file an issue.', + ); }).toErrorDev([ 'Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks', 'Do not call Hooks inside useEffect(...), useMemo(...), or other built-in Hooks',