From f0129fbe904294126635cdad6d47c6c73953b59a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 21 Jan 2019 20:36:13 +0000 Subject: [PATCH 1/6] Revert "Revert "Double-render function components with Hooks in DEV in StrictMode" (#14652)" This reverts commit 3fbebb2a0b3b806f428c4154700af8e75e561895. From cd3950364c26e4e24a6724428408da9c1c74e9e5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 21 Jan 2019 20:36:13 +0000 Subject: [PATCH 2/6] Revert "Revert "Disallow reading context during useMemo etc" (#14651)" This reverts commit 5fce6488ce1dc97e31515a47ff409d32ab722d65. --- .../src/ReactFiberClassComponent.js | 9 +- .../src/ReactFiberDispatcher.js | 2 +- .../react-reconciler/src/ReactFiberHooks.js | 29 ++++++- .../src/__tests__/ReactHooks-test.internal.js | 82 +++++++++++++++++++ 4 files changed, 110 insertions(+), 12 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 16a3e39fc31..3875371714f 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -20,7 +20,6 @@ import { import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import {isMounted} from 'react-reconciler/reflection'; import {get as getInstance, set as setInstance} from 'shared/ReactInstanceMap'; -import ReactSharedInternals from 'shared/ReactSharedInternals'; import shallowEqual from 'shared/shallowEqual'; import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; @@ -48,6 +47,7 @@ import { hasContextChanged, emptyContextObject, } from './ReactFiberContext'; +import {readContext} from './ReactFiberNewContext'; import { requestCurrentTime, computeExpirationForFiber, @@ -55,13 +55,6 @@ import { flushPassiveEffects, } from './ReactFiberScheduler'; -const ReactCurrentDispatcher = ReactSharedInternals.ReactCurrentDispatcher; - -function readContext(contextType: any): any { - const dispatcher = ReactCurrentDispatcher.current; - return dispatcher.readContext(contextType); -} - const fakeInternalInstance = {}; const isArray = Array.isArray; diff --git a/packages/react-reconciler/src/ReactFiberDispatcher.js b/packages/react-reconciler/src/ReactFiberDispatcher.js index ed2ed3b2f31..ad1f2b3e51c 100644 --- a/packages/react-reconciler/src/ReactFiberDispatcher.js +++ b/packages/react-reconciler/src/ReactFiberDispatcher.js @@ -7,8 +7,8 @@ * @flow */ -import {readContext} from './ReactFiberNewContext'; import { + readContext, useCallback, useContext, useEffect, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index cbd2a255bbd..916094187e4 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -14,7 +14,7 @@ import type {HookEffectTag} from './ReactHookEffectTags'; import {NoWork} from './ReactFiberExpirationTime'; import {enableHooks} from 'shared/ReactFeatureFlags'; -import {readContext} from './ReactFiberNewContext'; +import {readContext as readContextWithoutCheck} from './ReactFiberNewContext'; import { Update as UpdateEffect, Passive as PassiveEffect, @@ -284,7 +284,7 @@ export function resetHooks(): void { // This is used to reset the state of this module when a component throws. // It's also called inside mountIndeterminateComponent if we determine the - // component is a module-style component. + // component is a module-style component, and also in readContext() above. renderExpirationTime = NoWork; currentlyRenderingFiber = null; @@ -394,7 +394,7 @@ export function useContext( // Ensure we're in a function component (class components support only the // .unstable_read() form) resolveCurrentlyRenderingFiber(); - return readContext(context, observedBits); + return readContextWithoutCheck(context, observedBits); } export function useState( @@ -785,6 +785,29 @@ export function useMemo( return nextValue; } +export function readContext( + context: ReactContext, + observedBits: void | number | boolean, +): T { + // Forbid reading context inside Hooks. + // The outer check tells us whether we're inside a Hook like useMemo(). + // However, it would also be true if we're rendering a class. + if (currentlyRenderingFiber === null) { + // The inner check tells us we're currently in renderWithHooks() phase + // rather than, for example, in a class or a context consumer. + // Then we know it should be an error. + if (renderExpirationTime !== NoWork) { + invariant( + false, + 'Context can only be read inside the body of a component. ' + + 'If you read context inside a Hook like useMemo or useReducer, ' + + 'move the call directly into the component body.', + ); + } + } + return readContextWithoutCheck(context, observedBits); +} + function dispatchAction( fiber: Fiber, queue: UpdateQueue, diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 0008a27b148..32336667767 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -672,6 +672,88 @@ describe('ReactHooks', () => { expect(root.toJSON()).toEqual('123'); }); + it('throws when reading context inside useMemo', () => { + const {useMemo, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + return useMemo(() => { + return ReactCurrentDispatcher.current.readContext(ThemeContext); + }, []); + } + + expect(() => ReactTestRenderer.create()).toThrow( + 'Context can only be read inside the body of a component', + ); + }); + + it('throws when reading context inside useEffect', () => { + const {useEffect, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + useEffect(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }); + return null; + } + + const root = ReactTestRenderer.create(); + expect(() => root.update()).toThrow( + // The exact message doesn't matter, just make sure we don't allow this + "Cannot read property 'readContext' of null", + ); + }); + + it('throws when reading context inside useLayoutEffect', () => { + const {useLayoutEffect, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + useLayoutEffect(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }); + return null; + } + + expect(() => ReactTestRenderer.create()).toThrow( + // The exact message doesn't matter, just make sure we don't allow this + "Cannot read property 'readContext' of null", + ); + }); + + it('throws when reading context inside useReducer', () => { + const {useReducer, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + function App() { + useReducer( + () => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }, + null, + {}, + ); + return null; + } + + expect(() => ReactTestRenderer.create()).toThrow( + 'Context can only be read inside the body of a component.', + ); + }); + it('throws when calling hooks inside useReducer', () => { const {useReducer, useRef} = React; function App() { From 093c8a27e9a20d28d85dddc7b34254c29af67af6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 22 Jan 2019 14:50:36 +0000 Subject: [PATCH 3/6] Add extra passing test for an edge case Mentioned by @acdlite to watch out for --- .../src/__tests__/ReactHooks-test.internal.js | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 32336667767..69e436433af 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -690,6 +690,27 @@ describe('ReactHooks', () => { ); }); + it('throws when reading context inside useMemo after outside it', () => { + const {useMemo, createContext} = React; + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + const ThemeContext = createContext('light'); + let firstRead; + function App() { + firstRead = ReactCurrentDispatcher.current.readContext(ThemeContext); + return useMemo(() => { + return ReactCurrentDispatcher.current.readContext(ThemeContext); + }, []); + } + + expect(() => ReactTestRenderer.create()).toThrow( + 'Context can only be read inside the body of a component', + ); + expect(firstRead).toBe('light'); + }); + it('throws when reading context inside useEffect', () => { const {useEffect, createContext} = React; const ReactCurrentDispatcher = From 7cb26fbd25f82ff5098ee662b431ebe21dfb70e9 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 22 Jan 2019 14:52:22 +0000 Subject: [PATCH 4/6] More convoluted test --- .../src/__tests__/ReactHooks-test.internal.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 69e436433af..e73b566649b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -690,16 +690,18 @@ describe('ReactHooks', () => { ); }); - it('throws when reading context inside useMemo after outside it', () => { + it('throws when reading context inside useMemo after reading outside it', () => { const {useMemo, createContext} = React; const ReactCurrentDispatcher = React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED .ReactCurrentDispatcher; const ThemeContext = createContext('light'); - let firstRead; + let firstRead, secondRead; function App() { firstRead = ReactCurrentDispatcher.current.readContext(ThemeContext); + useMemo(() => {}); + secondRead = ReactCurrentDispatcher.current.readContext(ThemeContext); return useMemo(() => { return ReactCurrentDispatcher.current.readContext(ThemeContext); }, []); @@ -709,6 +711,7 @@ describe('ReactHooks', () => { 'Context can only be read inside the body of a component', ); expect(firstRead).toBe('light'); + expect(secondRead).toBe('light'); }); it('throws when reading context inside useEffect', () => { From cfbdf90d235c9f5ff8e07b24707f2d427ad1c4e4 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 22 Jan 2019 15:08:11 +0000 Subject: [PATCH 5/6] Don't rely on expirationTime Addresses @acdlite's concerns --- .../src/ReactFiberDispatcher.js | 2 +- .../react-reconciler/src/ReactFiberHooks.js | 41 +++++++------------ .../src/ReactFiberNewContext.js | 25 +++++++++++ .../src/__tests__/ReactHooks-test.internal.js | 6 +-- 4 files changed, 44 insertions(+), 30 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberDispatcher.js b/packages/react-reconciler/src/ReactFiberDispatcher.js index ad1f2b3e51c..ed2ed3b2f31 100644 --- a/packages/react-reconciler/src/ReactFiberDispatcher.js +++ b/packages/react-reconciler/src/ReactFiberDispatcher.js @@ -7,8 +7,8 @@ * @flow */ +import {readContext} from './ReactFiberNewContext'; import { - readContext, useCallback, useContext, useEffect, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 916094187e4..fd8c93253b1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -14,7 +14,11 @@ import type {HookEffectTag} from './ReactHookEffectTags'; import {NoWork} from './ReactFiberExpirationTime'; import {enableHooks} from 'shared/ReactFeatureFlags'; -import {readContext as readContextWithoutCheck} from './ReactFiberNewContext'; +import { + readContext, + stashContextDependencies, + unstashContextDependencies, +} from './ReactFiberNewContext'; import { Update as UpdateEffect, Passive as PassiveEffect, @@ -284,7 +288,7 @@ export function resetHooks(): void { // This is used to reset the state of this module when a component throws. // It's also called inside mountIndeterminateComponent if we determine the - // component is a module-style component, and also in readContext() above. + // component is a module-style component. renderExpirationTime = NoWork; currentlyRenderingFiber = null; @@ -394,7 +398,7 @@ export function useContext( // Ensure we're in a function component (class components support only the // .unstable_read() form) resolveCurrentlyRenderingFiber(); - return readContextWithoutCheck(context, observedBits); + return readContext(context, observedBits); } export function useState( @@ -443,8 +447,10 @@ export function useReducer( const action = update.action; // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; + stashContextDependencies(); newState = reducer(newState, action); currentlyRenderingFiber = fiber; + unstashContextDependencies(); update = update.next; } while (update !== null); @@ -515,8 +521,10 @@ export function useReducer( const action = update.action; // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; + stashContextDependencies(); newState = reducer(newState, action); currentlyRenderingFiber = fiber; + unstashContextDependencies(); } } prevUpdate = update; @@ -547,6 +555,7 @@ export function useReducer( } // Temporarily clear to forbid calling Hooks in a reducer. currentlyRenderingFiber = null; + stashContextDependencies(); // There's no existing queue, so this is the initial render. if (reducer === basicStateReducer) { // Special case for `useState`. @@ -557,6 +566,7 @@ export function useReducer( initialState = reducer(initialState, initialAction); } currentlyRenderingFiber = fiber; + unstashContextDependencies(); workInProgressHook.memoizedState = workInProgressHook.baseState = initialState; queue = workInProgressHook.queue = { last: null, @@ -779,35 +789,14 @@ export function useMemo( // Temporarily clear to forbid calling Hooks. currentlyRenderingFiber = null; + stashContextDependencies(); const nextValue = nextCreate(); currentlyRenderingFiber = fiber; + unstashContextDependencies(); workInProgressHook.memoizedState = [nextValue, nextDeps]; return nextValue; } -export function readContext( - context: ReactContext, - observedBits: void | number | boolean, -): T { - // Forbid reading context inside Hooks. - // The outer check tells us whether we're inside a Hook like useMemo(). - // However, it would also be true if we're rendering a class. - if (currentlyRenderingFiber === null) { - // The inner check tells us we're currently in renderWithHooks() phase - // rather than, for example, in a class or a context consumer. - // Then we know it should be an error. - if (renderExpirationTime !== NoWork) { - invariant( - false, - 'Context can only be read inside the body of a component. ' + - 'If you read context inside a Hook like useMemo or useReducer, ' + - 'move the call directly into the component body.', - ); - } - } - return readContextWithoutCheck(context, observedBits); -} - function dispatchAction( fiber: Fiber, queue: UpdateQueue, diff --git a/packages/react-reconciler/src/ReactFiberNewContext.js b/packages/react-reconciler/src/ReactFiberNewContext.js index 9687f40d7f1..cbba426499d 100644 --- a/packages/react-reconciler/src/ReactFiberNewContext.js +++ b/packages/react-reconciler/src/ReactFiberNewContext.js @@ -52,12 +52,37 @@ let currentlyRenderingFiber: Fiber | null = null; let lastContextDependency: ContextDependency | null = null; let lastContextWithAllBitsObserved: ReactContext | null = null; +// We stash the variables above before entering user code in Hooks. +let stashedCurrentlyRenderingFiber: Fiber | null = null; +let stashedLastContextDependency: ContextDependency | null = null; +let stashedLastContextWithAllBitsObserved: ReactContext | null = null; + export function resetContextDependences(): void { // This is called right before React yields execution, to ensure `readContext` // cannot be called outside the render phase. currentlyRenderingFiber = null; lastContextDependency = null; lastContextWithAllBitsObserved = null; + + stashedCurrentlyRenderingFiber = null; + stashedLastContextDependency = null; + stashedLastContextWithAllBitsObserved = null; +} + +export function stashContextDependencies(): void { + stashedCurrentlyRenderingFiber = currentlyRenderingFiber; + stashedLastContextDependency = lastContextDependency; + stashedLastContextWithAllBitsObserved = lastContextWithAllBitsObserved; + + currentlyRenderingFiber = null; + lastContextDependency = null; + lastContextWithAllBitsObserved = null; +} + +export function unstashContextDependencies(): void { + currentlyRenderingFiber = stashedCurrentlyRenderingFiber; + lastContextDependency = stashedLastContextDependency; + lastContextWithAllBitsObserved = stashedLastContextWithAllBitsObserved; } export function pushProvider(providerFiber: Fiber, nextValue: T): void { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index e73b566649b..0eae5155e77 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -686,7 +686,7 @@ describe('ReactHooks', () => { } expect(() => ReactTestRenderer.create()).toThrow( - 'Context can only be read inside the body of a component', + 'Context can only be read while React is rendering', ); }); @@ -708,7 +708,7 @@ describe('ReactHooks', () => { } expect(() => ReactTestRenderer.create()).toThrow( - 'Context can only be read inside the body of a component', + 'Context can only be read while React is rendering', ); expect(firstRead).toBe('light'); expect(secondRead).toBe('light'); @@ -774,7 +774,7 @@ describe('ReactHooks', () => { } expect(() => ReactTestRenderer.create()).toThrow( - 'Context can only be read inside the body of a component.', + 'Context can only be read while React is rendering', ); }); From 223960e1ea05c824515bfe7ee363cf5becbb0f4d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 23 Jan 2019 15:39:19 +0000 Subject: [PATCH 6/6] Edge case: forbid readContext() during eager reducer --- .../react-reconciler/src/ReactFiberHooks.js | 6 ++++ .../src/__tests__/ReactHooks-test.internal.js | 35 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index fd8c93253b1..538748cd0a2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -887,7 +887,13 @@ function dispatchAction( if (eagerReducer !== null) { try { const currentState: S = (queue.eagerState: any); + // Temporarily clear to forbid calling Hooks in a reducer. + let maybeFiber = currentlyRenderingFiber; // Note: likely null now unlike `fiber` + currentlyRenderingFiber = null; + stashContextDependencies(); const eagerState = eagerReducer(currentState, action); + currentlyRenderingFiber = maybeFiber; + unstashContextDependencies(); // Stash the eagerly computed state, and the reducer used to compute // it, on the update object. If the reducer hasn't changed by the // time we enter the render phase, then the eager state can be used diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 0eae5155e77..b8e7e8d7d8a 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -778,6 +778,41 @@ describe('ReactHooks', () => { ); }); + // Edge case. + it('throws when reading context inside eager useReducer', () => { + const {useState, createContext} = React; + const ThemeContext = createContext('light'); + + const ReactCurrentDispatcher = + React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED + .ReactCurrentDispatcher; + + let _setState; + function Fn() { + const [, setState] = useState(0); + _setState = setState; + return null; + } + + class Cls extends React.Component { + render() { + _setState(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }); + return null; + } + } + + expect(() => + ReactTestRenderer.create( + + + + , + ), + ).toThrow('Context can only be read while React is rendering'); + }); + it('throws when calling hooks inside useReducer', () => { const {useReducer, useRef} = React; function App() {