From 00c01a986de08cbd9d7253e2f624e6baf45b823f Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 14 Jan 2019 07:27:49 +0000 Subject: [PATCH 01/13] hooks inside useMemo/.memo - failing tests --- .../src/__tests__/ReactHooks-test.internal.js | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 709d819d6713..bdfc6e54ede1 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -92,4 +92,28 @@ describe('ReactHooks', () => { const root = ReactTestRenderer.create(); expect(root.toJSON()).toMatchSnapshot(); }); + + it("throws when calling hooks inside .memo's compare function", () => { + const {useState} = React; + function App() { + return null; + } + App = React.memo(App, () => { + useState(0); + return false; + }); + expect(() => ReactTestRenderer.create()).toThrow(); + }); + + it("throws when calling hooks inside useMemo", () => { + const {useMemo, useState} = React; + function App() { + const value = useMemo(() => { + useState(0) + return 123 + }); + return null; + } + expect(() => ReactTestRenderer.create()).toThrow(); + }); }); From 59eed66c9ded1c4c10155e419379b4fabc3e5526 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 16 Jan 2019 14:00:50 +0000 Subject: [PATCH 02/13] throw an error when using hooks inside useMemo --- packages/react-reconciler/src/ReactFiberHooks.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index b859f4067b6a..48d33cb0523a 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -109,9 +109,11 @@ let renderPhaseUpdates: Map, Update> | null = null; let numberOfReRenders: number = 0; const RE_RENDER_LIMIT = 25; +let isMemoCreatingNextValue = false + function resolveCurrentlyRenderingFiber(): Fiber { invariant( - currentlyRenderingFiber !== null, + currentlyRenderingFiber !== null && isMemoCreatingNextValue === false , 'Hooks can only be called inside the body of a function component.', ); return currentlyRenderingFiber; @@ -626,8 +628,9 @@ export function useMemo( return prevState[0]; } } - + isMemoCreatingNextValue = true const nextValue = nextCreate(); + isMemoCreatingNextValue = false workInProgressHook.memoizedState = [nextValue, nextInputs]; return nextValue; } From f36027465128798b005ce5a38ec59abf8c0e6e56 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 16 Jan 2019 15:56:56 +0000 Subject: [PATCH 03/13] throw when using hooks inside .memo's compare fn --- .../src/ReactFiberBeginWork.js | 10 ++++++- .../react-reconciler/src/ReactFiberHooks.js | 15 ++++++---- .../src/__tests__/ReactHooks-test.internal.js | 28 +++++++++++++------ 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 988ee30f5fc6..a08a99cb6c98 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -90,7 +90,12 @@ import { prepareToReadContext, calculateChangedBits, } from './ReactFiberNewContext'; -import {prepareToUseHooks, finishHooks, resetHooks} from './ReactFiberHooks'; +import { + prepareToUseHooks, + pauseHooks, + finishHooks, + resetHooks, +} from './ReactFiberHooks'; import {stopProfilerTimerIfRunning} from './ReactProfilerTimer'; import { getMaskedContext, @@ -342,13 +347,16 @@ function updateMemoComponent( // Default to shallow comparison let compare = Component.compare; compare = compare !== null ? compare : shallowEqual; + pauseHooks(true); if (compare(prevProps, nextProps) && current.ref === workInProgress.ref) { + pauseHooks(false); return bailoutOnAlreadyFinishedWork( current, workInProgress, renderExpirationTime, ); } + pauseHooks(false); } // React DevTools reads this flag. workInProgress.effectTag |= PerformedWork; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 48d33cb0523a..97e3a8b197e9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -108,17 +108,21 @@ let renderPhaseUpdates: Map, Update> | null = null; // Counter to prevent infinite loops. let numberOfReRenders: number = 0; const RE_RENDER_LIMIT = 25; - -let isMemoCreatingNextValue = false +// Whether we've 'paused' running hooks +let areHooksPaused = false function resolveCurrentlyRenderingFiber(): Fiber { invariant( - currentlyRenderingFiber !== null && isMemoCreatingNextValue === false , + currentlyRenderingFiber !== null && areHooksPaused === false, 'Hooks can only be called inside the body of a function component.', ); return currentlyRenderingFiber; } +export function pauseHooks(pause:boolean){ + areHooksPaused = pause +} + export function prepareToUseHooks( current: Fiber | null, workInProgress: Fiber, @@ -129,6 +133,7 @@ export function prepareToUseHooks( } renderExpirationTime = nextRenderExpirationTime; currentlyRenderingFiber = workInProgress; + pauseHooks(false) firstCurrentHook = current !== null ? current.memoizedState : null; // The following should have already been reset @@ -628,9 +633,9 @@ export function useMemo( return prevState[0]; } } - isMemoCreatingNextValue = true + pauseHooks(true) const nextValue = nextCreate(); - isMemoCreatingNextValue = false + pauseHooks(false) workInProgressHook.memoizedState = [nextValue, nextInputs]; return nextValue; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index bdfc6e54ede1..5f1d3804aa78 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -94,26 +94,38 @@ describe('ReactHooks', () => { }); it("throws when calling hooks inside .memo's compare function", () => { - const {useState} = React; + const {useState} = React; function App() { + useState(0); return null; } - App = React.memo(App, () => { + const MemoApp = React.memo(App, () => { useState(0); return false; }); - expect(() => ReactTestRenderer.create()).toThrow(); + + const root = ReactTestRenderer.create(); + // trying to render again should trigger comparison and throw + expect(() => root.update()).toThrow(); + // the next round, it does a fresh mount, so should render + expect(() => root.update()).not.toThrow(); + // and then again, fail + expect(() => root.update()).toThrow(); }); - it("throws when calling hooks inside useMemo", () => { + it('throws when calling hooks inside useMemo', () => { const {useMemo, useState} = React; function App() { const value = useMemo(() => { - useState(0) - return 123 + useState(0); + return 123; }); return null; - } + } expect(() => ReactTestRenderer.create()).toThrow(); - }); + }); + + // todo - + // it can't read from context inside useMemo / .memo + // ^ the above tests, but for the server renderer }); From 4415d590ecfb2702eeef082e156fc25deb800ff7 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 16 Jan 2019 18:50:33 +0000 Subject: [PATCH 04/13] faster/better/stronger --- .../src/server/ReactPartialRendererHooks.js | 11 ++++++- .../src/ReactFiberBeginWork.js | 10 +------ .../react-reconciler/src/ReactFiberHooks.js | 24 +++++++-------- .../src/__tests__/ReactHooks-test.internal.js | 29 +++++++++++++++---- 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index d5ca03dee81a..3759a9323d20 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -229,11 +229,14 @@ export function useReducer( } } +// a variable to store current component while we compare memo values +let cachedCurrentlyRenderingComponent = null; + function useMemo( nextCreate: () => T, inputs: Array | void | null, ): T { - currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); + cachedCurrentlyRenderingComponent = currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); workInProgressHook = createWorkInProgressHook(); const nextInputs = @@ -250,7 +253,13 @@ function useMemo( } } + // null the reference to the component + currentlyRenderingComponent = null; + // now, it'll throw if you try to call a hook inside nextCreate const nextValue = nextCreate(); + // restore the current component + currentlyRenderingComponent = cachedCurrentlyRenderingComponent; + workInProgressHook.memoizedState = [nextValue, nextInputs]; return nextValue; } diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index a08a99cb6c98..988ee30f5fc6 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -90,12 +90,7 @@ import { prepareToReadContext, calculateChangedBits, } from './ReactFiberNewContext'; -import { - prepareToUseHooks, - pauseHooks, - finishHooks, - resetHooks, -} from './ReactFiberHooks'; +import {prepareToUseHooks, finishHooks, resetHooks} from './ReactFiberHooks'; import {stopProfilerTimerIfRunning} from './ReactProfilerTimer'; import { getMaskedContext, @@ -347,16 +342,13 @@ function updateMemoComponent( // Default to shallow comparison let compare = Component.compare; compare = compare !== null ? compare : shallowEqual; - pauseHooks(true); if (compare(prevProps, nextProps) && current.ref === workInProgress.ref) { - pauseHooks(false); return bailoutOnAlreadyFinishedWork( current, workInProgress, renderExpirationTime, ); } - pauseHooks(false); } // React DevTools reads this flag. workInProgress.effectTag |= PerformedWork; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 97e3a8b197e9..f4dcbb054a82 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -108,21 +108,14 @@ let renderPhaseUpdates: Map, Update> | null = null; // Counter to prevent infinite loops. let numberOfReRenders: number = 0; const RE_RENDER_LIMIT = 25; -// Whether we've 'paused' running hooks -let areHooksPaused = false - function resolveCurrentlyRenderingFiber(): Fiber { invariant( - currentlyRenderingFiber !== null && areHooksPaused === false, + currentlyRenderingFiber !== null, 'Hooks can only be called inside the body of a function component.', ); return currentlyRenderingFiber; } -export function pauseHooks(pause:boolean){ - areHooksPaused = pause -} - export function prepareToUseHooks( current: Fiber | null, workInProgress: Fiber, @@ -133,7 +126,6 @@ export function prepareToUseHooks( } renderExpirationTime = nextRenderExpirationTime; currentlyRenderingFiber = workInProgress; - pauseHooks(false) firstCurrentHook = current !== null ? current.memoizedState : null; // The following should have already been reset @@ -616,11 +608,14 @@ export function useCallback( return callback; } +// a variable to store current component while we compare memo values +let cachedCurrentlyRenderingFiber = null; + export function useMemo( nextCreate: () => T, inputs: Array | void | null, ): T { - currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + cachedCurrentlyRenderingFiber = currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); workInProgressHook = createWorkInProgressHook(); const nextInputs = @@ -633,9 +628,14 @@ export function useMemo( return prevState[0]; } } - pauseHooks(true) + + // null the reference to the component + currentlyRenderingFiber = null; + // now, it'll throw if you try to call a hook inside nextCreate const nextValue = nextCreate(); - pauseHooks(false) + // restore the current fiber + currentlyRenderingFiber = cachedCurrentlyRenderingFiber; + workInProgressHook.memoizedState = [nextValue, nextInputs]; return nextValue; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 5f1d3804aa78..58816a580a73 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -107,25 +107,42 @@ describe('ReactHooks', () => { const root = ReactTestRenderer.create(); // trying to render again should trigger comparison and throw expect(() => root.update()).toThrow(); - // the next round, it does a fresh mount, so should render + // the next round, it does a fresh mount, so should render expect(() => root.update()).not.toThrow(); // and then again, fail - expect(() => root.update()).toThrow(); + expect(() => root.update()).toThrow(); }); it('throws when calling hooks inside useMemo', () => { const {useMemo, useState} = React; function App() { - const value = useMemo(() => { + useMemo(() => { useState(0); return 123; }); return null; } expect(() => ReactTestRenderer.create()).toThrow(); - }); + }); + + it('cannot useContext inside useMemo', () => { + const {useMemo, useContext} = React; + const Ctx = React.createContext(); + function App() { + useMemo(() => { + useContext(Ctx); + return 123; + }); + return null; + } + + expect(() => ReactTestRenderer.create( + + + , + )).toThrow(); + }); - // todo - - // it can't read from context inside useMemo / .memo + // todo - // ^ the above tests, but for the server renderer }); From ec175a326b70becd6cffe26809da21177fc7e149 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 17 Jan 2019 21:38:33 +0000 Subject: [PATCH 05/13] same logic for useReducer, tests for the server, etc --- ...DOMServerIntegrationHooks-test.internal.js | 46 ++++++++++++++ .../src/server/ReactPartialRendererHooks.js | 17 +++--- .../react-reconciler/src/ReactFiberHooks.js | 20 +++--- .../src/__tests__/ReactHooks-test.internal.js | 61 ++++++++++++------- 4 files changed, 108 insertions(+), 36 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index 2e585a1373a2..69774705c37b 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -417,6 +417,52 @@ describe('ReactDOMServerHooks', () => { expect(domNode.textContent).toEqual('HELLO, WORLD.'); }, ); + + itThrowsWhenRendering( + 'a hook inside useMemo', + async render => { + function App() { + useMemo(() => { + useState(); + return 0; + }); + return null; + } + return render(); + }, + 'Hooks can only be called inside the body of a function component.', + ); + + itThrowsWhenRendering( + 'a hook inside useReducer', + async render => { + function App() { + const [value, dispatch] = useReducer((state, action) => { + useRef(0); + return state; + }, 0); + dispatch('foo'); + return value; + } + return render(); + }, + 'Hooks can only be called inside the body of a function component.', + ); + + itThrowsWhenRendering( + 'a hook inside useState', + async render => { + const {useState, useRef} = React; + function App() { + useState(() => { + useRef(0); + return 0; + }); + } + return render(); + }, + 'Hooks can only be called inside the body of a function component.', + ); }); describe('useRef', () => { diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 3759a9323d20..2b2b883995c4 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -177,7 +177,7 @@ export function useReducer( initialState: S, initialAction: A | void | null, ): [S, Dispatch] { - currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); + let cachedCurrentlyRenderingComponent = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent()); workInProgressHook = createWorkInProgressHook(); if (isReRender) { // This is a re-render. Apply the new render phase updates to the previous @@ -196,7 +196,12 @@ export function useReducer( // priority because it will always be the same as the current // render's. const action = update.action; + + currentlyRenderingComponent = null; + // now, it'll throw if you try to call a hook inside the reducer newState = reducer(newState, action); + // restore the current component + currentlyRenderingComponent = cachedCurrentlyRenderingComponent; update = update.next; } while (update !== null); @@ -207,6 +212,7 @@ export function useReducer( } return [workInProgressHook.memoizedState, dispatch]; } else { + currentlyRenderingComponent = null; if (reducer === basicStateReducer) { // Special case for `useState`. if (typeof initialState === 'function') { @@ -215,6 +221,7 @@ export function useReducer( } else if (initialAction !== undefined && initialAction !== null) { initialState = reducer(initialState, initialAction); } + currentlyRenderingComponent = cachedCurrentlyRenderingComponent; workInProgressHook.memoizedState = initialState; const queue: UpdateQueue = (workInProgressHook.queue = { last: null, @@ -229,14 +236,11 @@ export function useReducer( } } -// a variable to store current component while we compare memo values -let cachedCurrentlyRenderingComponent = null; - function useMemo( nextCreate: () => T, inputs: Array | void | null, ): T { - cachedCurrentlyRenderingComponent = currentlyRenderingComponent = resolveCurrentlyRenderingComponent(); + let cachedCurrentlyRenderingComponent = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent()); workInProgressHook = createWorkInProgressHook(); const nextInputs = @@ -253,9 +257,8 @@ function useMemo( } } - // null the reference to the component currentlyRenderingComponent = null; - // now, it'll throw if you try to call a hook inside nextCreate + // now, it'll throw if you try to call a hook inside nextCreate() const nextValue = nextCreate(); // restore the current component currentlyRenderingComponent = cachedCurrentlyRenderingComponent; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index f4dcbb054a82..0a438230ae18 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -344,7 +344,7 @@ export function useReducer( initialState: S, initialAction: A | void | null, ): [S, Dispatch] { - currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); workInProgressHook = createWorkInProgressHook(); let queue: UpdateQueue | null = (workInProgressHook.queue: any); if (queue !== null) { @@ -365,7 +365,12 @@ export function useReducer( // priority because it will always be the same as the current // render's. const action = update.action; + currentlyRenderingFiber = null; + + // now, it'll throw if you try to call a hook inside the reducer newState = reducer(newState, action); + // restore the current component + currentlyRenderingFiber = cachedCurrentlyRenderingFiber; update = update.next; } while (update !== null); @@ -428,7 +433,9 @@ export function useReducer( } else { // Process this update. const action = update.action; + currentlyRenderingFiber = null; newState = reducer(newState, action); + currentlyRenderingFiber = cachedCurrentlyRenderingFiber; } prevUpdate = update; update = update.next; @@ -447,7 +454,7 @@ export function useReducer( const dispatch: Dispatch = (queue.dispatch: any); return [workInProgressHook.memoizedState, dispatch]; } - + currentlyRenderingFiber = null; // There's no existing queue, so this is the initial render. if (reducer === basicStateReducer) { // Special case for `useState`. @@ -457,6 +464,7 @@ export function useReducer( } else if (initialAction !== undefined && initialAction !== null) { initialState = reducer(initialState, initialAction); } + currentlyRenderingFiber = cachedCurrentlyRenderingFiber; workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; queue = workInProgressHook.queue = { last: null, @@ -608,14 +616,11 @@ export function useCallback( return callback; } -// a variable to store current component while we compare memo values -let cachedCurrentlyRenderingFiber = null; - export function useMemo( nextCreate: () => T, inputs: Array | void | null, ): T { - cachedCurrentlyRenderingFiber = currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); + let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); workInProgressHook = createWorkInProgressHook(); const nextInputs = @@ -628,8 +633,7 @@ export function useMemo( return prevState[0]; } } - - // null the reference to the component + currentlyRenderingFiber = null; // now, it'll throw if you try to call a hook inside nextCreate const nextValue = nextCreate(); diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 58816a580a73..1b4a2d6d691b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -17,6 +17,9 @@ let ReactFeatureFlags; let ReactTestRenderer; let ReactDOMServer; +const OutsideHookScopeError = + 'Hooks can only be called inside the body of a function component'; + // Additional tests can be found in ReactHooksWithNoopRenderer. Plan is to // gradually migrate those to this file. describe('ReactHooks', () => { @@ -106,11 +109,11 @@ describe('ReactHooks', () => { const root = ReactTestRenderer.create(); // trying to render again should trigger comparison and throw - expect(() => root.update()).toThrow(); + expect(() => root.update()).toThrow(OutsideHookScopeError); // the next round, it does a fresh mount, so should render - expect(() => root.update()).not.toThrow(); + expect(() => root.update()).not.toThrow(OutsideHookScopeError); // and then again, fail - expect(() => root.update()).toThrow(); + expect(() => root.update()).toThrow(OutsideHookScopeError); }); it('throws when calling hooks inside useMemo', () => { @@ -122,27 +125,43 @@ describe('ReactHooks', () => { }); return null; } - expect(() => ReactTestRenderer.create()).toThrow(); + + function Simple() { + useState(0); + return 123; + } + let root = ReactTestRenderer.create(null); + expect(() => root.update()).toThrow(OutsideHookScopeError); + // we want to assure that no hook machinery has broken, + // so we listen for all errors on the next line + expect(() => root.update()).not.toThrow(); }); - - it('cannot useContext inside useMemo', () => { - const {useMemo, useContext} = React; - const Ctx = React.createContext(); + + it('throws when calling hooks inside useReducer', () => { + const {useReducer, useRef} = React; function App() { - useMemo(() => { - useContext(Ctx); - return 123; - }); - return null; + const [value, dispatch] = useReducer((state, action) => { + useRef(0); + return state; + }, 0); + dispatch('foo'); + return value; } - - expect(() => ReactTestRenderer.create( - - - , - )).toThrow(); + expect(() => ReactTestRenderer.create()).toThrow( + OutsideHookScopeError, + ); }); - // todo - - // ^ the above tests, but for the server renderer + it("throws when calling hooks inside useState's initialize function", () => { + const {useState, useRef} = React; + function App() { + useState(() => { + useRef(0); + return 0; + }); + } + expect(() => ReactTestRenderer.create()).toThrow( + OutsideHookScopeError, + ); + }); }); From dacce6a3e6f9d345bea081c7db424ee9833f0ce9 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 17 Jan 2019 21:43:11 +0000 Subject: [PATCH 06/13] Update ReactDOMServerIntegrationHooks-test.internal.js ack lint --- .../__tests__/ReactDOMServerIntegrationHooks-test.internal.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index 69774705c37b..f923c075209a 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js @@ -452,7 +452,6 @@ describe('ReactDOMServerHooks', () => { itThrowsWhenRendering( 'a hook inside useState', async render => { - const {useState, useRef} = React; function App() { useState(() => { useRef(0); From 78b3c6a1263705886ac58c89bfabb0644b25c162 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 17 Jan 2019 22:51:07 +0000 Subject: [PATCH 07/13] nits --- .../src/server/ReactPartialRendererHooks.js | 19 ++++------ .../react-reconciler/src/ReactFiberHooks.js | 21 +++++------ .../src/__tests__/ReactHooks-test.internal.js | 37 +++++++++++-------- 3 files changed, 38 insertions(+), 39 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 2b2b883995c4..a3f890682cb0 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -177,7 +177,7 @@ export function useReducer( initialState: S, initialAction: A | void | null, ): [S, Dispatch] { - let cachedCurrentlyRenderingComponent = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent()); + let component = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent()); workInProgressHook = createWorkInProgressHook(); if (isReRender) { // This is a re-render. Apply the new render phase updates to the previous @@ -196,12 +196,10 @@ export function useReducer( // priority because it will always be the same as the current // render's. const action = update.action; - + // Temporarily; clear to forbid calling Hooks. currentlyRenderingComponent = null; - // now, it'll throw if you try to call a hook inside the reducer newState = reducer(newState, action); - // restore the current component - currentlyRenderingComponent = cachedCurrentlyRenderingComponent; + currentlyRenderingComponent = component; update = update.next; } while (update !== null); @@ -221,7 +219,7 @@ export function useReducer( } else if (initialAction !== undefined && initialAction !== null) { initialState = reducer(initialState, initialAction); } - currentlyRenderingComponent = cachedCurrentlyRenderingComponent; + currentlyRenderingComponent = component; workInProgressHook.memoizedState = initialState; const queue: UpdateQueue = (workInProgressHook.queue = { last: null, @@ -240,7 +238,7 @@ function useMemo( nextCreate: () => T, inputs: Array | void | null, ): T { - let cachedCurrentlyRenderingComponent = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent()); + let component = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent()); workInProgressHook = createWorkInProgressHook(); const nextInputs = @@ -256,13 +254,10 @@ function useMemo( return prevState[0]; } } - + // Temporarily clear to forbid calling Hooks. currentlyRenderingComponent = null; - // now, it'll throw if you try to call a hook inside nextCreate() const nextValue = nextCreate(); - // restore the current component - currentlyRenderingComponent = cachedCurrentlyRenderingComponent; - + currentlyRenderingComponent = component; workInProgressHook.memoizedState = [nextValue, nextInputs]; return nextValue; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 0a438230ae18..ba0060b4d576 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -108,6 +108,7 @@ let renderPhaseUpdates: Map, Update> | null = null; // Counter to prevent infinite loops. let numberOfReRenders: number = 0; const RE_RENDER_LIMIT = 25; + function resolveCurrentlyRenderingFiber(): Fiber { invariant( currentlyRenderingFiber !== null, @@ -344,7 +345,7 @@ export function useReducer( initialState: S, initialAction: A | void | null, ): [S, Dispatch] { - let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); + let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); workInProgressHook = createWorkInProgressHook(); let queue: UpdateQueue | null = (workInProgressHook.queue: any); if (queue !== null) { @@ -365,12 +366,11 @@ export function useReducer( // priority because it will always be the same as the current // render's. const action = update.action; + // Temporarily clear to forbid calling nested Hooks. currentlyRenderingFiber = null; - // now, it'll throw if you try to call a hook inside the reducer newState = reducer(newState, action); - // restore the current component - currentlyRenderingFiber = cachedCurrentlyRenderingFiber; + currentlyRenderingFiber = fiber; update = update.next; } while (update !== null); @@ -435,7 +435,7 @@ export function useReducer( const action = update.action; currentlyRenderingFiber = null; newState = reducer(newState, action); - currentlyRenderingFiber = cachedCurrentlyRenderingFiber; + currentlyRenderingFiber = fiber; } prevUpdate = update; update = update.next; @@ -464,7 +464,7 @@ export function useReducer( } else if (initialAction !== undefined && initialAction !== null) { initialState = reducer(initialState, initialAction); } - currentlyRenderingFiber = cachedCurrentlyRenderingFiber; + currentlyRenderingFiber = fiber; workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; queue = workInProgressHook.queue = { last: null, @@ -620,7 +620,7 @@ export function useMemo( nextCreate: () => T, inputs: Array | void | null, ): T { - let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); + let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); workInProgressHook = createWorkInProgressHook(); const nextInputs = @@ -633,13 +633,10 @@ export function useMemo( return prevState[0]; } } - + // Temporarily clear to forbid calling Hooks. currentlyRenderingFiber = null; - // now, it'll throw if you try to call a hook inside nextCreate const nextValue = nextCreate(); - // restore the current fiber - currentlyRenderingFiber = cachedCurrentlyRenderingFiber; - + currentlyRenderingFiber = fiber; workInProgressHook.memoizedState = [nextValue, nextInputs]; return nextValue; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 1b4a2d6d691b..9ec16734de67 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -17,9 +17,6 @@ let ReactFeatureFlags; let ReactTestRenderer; let ReactDOMServer; -const OutsideHookScopeError = - 'Hooks can only be called inside the body of a function component'; - // Additional tests can be found in ReactHooksWithNoopRenderer. Plan is to // gradually migrate those to this file. describe('ReactHooks', () => { @@ -109,11 +106,17 @@ describe('ReactHooks', () => { const root = ReactTestRenderer.create(); // trying to render again should trigger comparison and throw - expect(() => root.update()).toThrow(OutsideHookScopeError); + expect(() => root.update()).toThrow( + 'Hooks can only be called inside the body of a function component', + ); // the next round, it does a fresh mount, so should render - expect(() => root.update()).not.toThrow(OutsideHookScopeError); + expect(() => root.update()).not.toThrow( + 'Hooks can only be called inside the body of a function component', + ); // and then again, fail - expect(() => root.update()).toThrow(OutsideHookScopeError); + expect(() => root.update()).toThrow( + 'Hooks can only be called inside the body of a function component', + ); }); it('throws when calling hooks inside useMemo', () => { @@ -121,20 +124,24 @@ describe('ReactHooks', () => { function App() { useMemo(() => { useState(0); - return 123; + return 1; }); return null; } function Simple() { - useState(0); - return 123; + const [value] = useState(123); + return value; } let root = ReactTestRenderer.create(null); - expect(() => root.update()).toThrow(OutsideHookScopeError); - // we want to assure that no hook machinery has broken, - // so we listen for all errors on the next line - expect(() => root.update()).not.toThrow(); + expect(() => root.update()).toThrow( + 'Hooks can only be called inside the body of a function component', + ); + + // we want to assure that no hook machinery has broken + // so we render a fresh component with a hook just to be sure + root.update(); + expect(root.toJSON()).toEqual('123'); }); it('throws when calling hooks inside useReducer', () => { @@ -148,7 +155,7 @@ describe('ReactHooks', () => { return value; } expect(() => ReactTestRenderer.create()).toThrow( - OutsideHookScopeError, + 'Hooks can only be called inside the body of a function component', ); }); @@ -161,7 +168,7 @@ describe('ReactHooks', () => { }); } expect(() => ReactTestRenderer.create()).toThrow( - OutsideHookScopeError, + 'Hooks can only be called inside the body of a function component', ); }); }); From fdd03f3b273a9a0cec9284bf7231ff33c562e585 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 17 Jan 2019 22:54:20 +0000 Subject: [PATCH 08/13] whitespace --- packages/react-dom/src/server/ReactPartialRendererHooks.js | 1 + packages/react-reconciler/src/ReactFiberHooks.js | 2 ++ 2 files changed, 3 insertions(+) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index a3f890682cb0..4bfb4a70c489 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -254,6 +254,7 @@ function useMemo( return prevState[0]; } } + // Temporarily clear to forbid calling Hooks. currentlyRenderingComponent = null; const nextValue = nextCreate(); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index ba0060b4d576..877bbc630422 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -454,6 +454,7 @@ export function useReducer( const dispatch: Dispatch = (queue.dispatch: any); return [workInProgressHook.memoizedState, dispatch]; } + currentlyRenderingFiber = null; // There's no existing queue, so this is the initial render. if (reducer === basicStateReducer) { @@ -633,6 +634,7 @@ export function useMemo( return prevState[0]; } } + // Temporarily clear to forbid calling Hooks. currentlyRenderingFiber = null; const nextValue = nextCreate(); From 05aa71e850e7532d010d884ad288fefff2b15cd5 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 17 Jan 2019 22:55:56 +0000 Subject: [PATCH 09/13] whitespace --- packages/react-dom/src/server/ReactPartialRendererHooks.js | 2 +- packages/react-reconciler/src/ReactFiberHooks.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 4bfb4a70c489..037c6c673d74 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -254,7 +254,7 @@ function useMemo( return prevState[0]; } } - + // Temporarily clear to forbid calling Hooks. currentlyRenderingComponent = null; const nextValue = nextCreate(); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 877bbc630422..d6435e401d8a 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -634,7 +634,7 @@ export function useMemo( return prevState[0]; } } - + // Temporarily clear to forbid calling Hooks. currentlyRenderingFiber = null; const nextValue = nextCreate(); From aec0473c121b43c0a95a8f393bb698a4e5774fdf Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 17 Jan 2019 23:22:41 +0000 Subject: [PATCH 10/13] stray semi --- packages/react-dom/src/server/ReactPartialRendererHooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/server/ReactPartialRendererHooks.js b/packages/react-dom/src/server/ReactPartialRendererHooks.js index 233bd817e1e5..b89e9899868b 100644 --- a/packages/react-dom/src/server/ReactPartialRendererHooks.js +++ b/packages/react-dom/src/server/ReactPartialRendererHooks.js @@ -253,7 +253,7 @@ export function useReducer( // priority because it will always be the same as the current // render's. const action = update.action; - // Temporarily; clear to forbid calling Hooks. + // Temporarily clear to forbid calling Hooks. currentlyRenderingComponent = null; newState = reducer(newState, action); currentlyRenderingComponent = component; From a954f6ce8aae13b530b273b7843283ef42204be2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 18 Jan 2019 00:23:22 +0000 Subject: [PATCH 11/13] Tweak comment --- packages/react-reconciler/src/ReactFiberHooks.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 647fc7bb1070..d3da9f3ae0ea 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -441,9 +441,8 @@ export function useReducer( // priority because it will always be the same as the current // render's. const action = update.action; - // Temporarily clear to forbid calling nested Hooks. + // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; - // now, it'll throw if you try to call a hook inside the reducer newState = reducer(newState, action); currentlyRenderingFiber = fiber; update = update.next; From 49aa7002a07c73f8df5074e71f7e7592437a32ef Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 18 Jan 2019 00:41:39 +0000 Subject: [PATCH 12/13] stray unmatched fiber reset --- packages/react-reconciler/src/ReactFiberHooks.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 647fc7bb1070..34769abf7e00 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -513,10 +513,11 @@ export function useReducer( // current reducer, we can use the eagerly computed state. newState = ((update.eagerState: any): S); } else { + currentlyRenderingFiber = null; const action = update.action; newState = reducer(newState, action); + currentlyRenderingFiber = fiber; } - currentlyRenderingFiber = fiber; } prevUpdate = update; update = update.next; From ced49782e2888d4c7ed38390c117c86ffc668966 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 18 Jan 2019 01:21:53 +0000 Subject: [PATCH 13/13] nit --- packages/react-reconciler/src/ReactFiberHooks.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8b1f9563f2a6..567d286029b2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -512,8 +512,9 @@ export function useReducer( // current reducer, we can use the eagerly computed state. newState = ((update.eagerState: any): S); } else { - currentlyRenderingFiber = null; const action = update.action; + // Temporarily clear to forbid calling Hooks in a reducer. + currentlyRenderingFiber = null; newState = reducer(newState, action); currentlyRenderingFiber = fiber; } @@ -544,7 +545,7 @@ export function useReducer( const dispatch: Dispatch = (queue.dispatch: any); return [workInProgressHook.memoizedState, dispatch]; } - + // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; // There's no existing queue, so this is the initial render. if (reducer === basicStateReducer) {