Skip to content
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

Throw an error when using hooks inside useMemo, or .memo's comparison function #14608

Merged
merged 16 commits into from
Jan 18, 2019
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,51 @@ 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(<App />);
},
'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(<App />);
},
'Hooks can only be called inside the body of a function component.',
);

itThrowsWhenRendering(
'a hook inside useState',
async render => {
function App() {
useState(() => {
useRef(0);
return 0;
});
}
return render(<App />);
},
'Hooks can only be called inside the body of a function component.',
);
});

describe('useRef', () => {
Expand Down
16 changes: 14 additions & 2 deletions packages/react-dom/src/server/ReactPartialRendererHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export function useReducer<S, A>(
initialState: S,
initialAction: A | void | null,
): [S, Dispatch<A>] {
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
let cachedCurrentlyRenderingComponent = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent());
workInProgressHook = createWorkInProgressHook();
if (isReRender) {
// This is a re-render. Apply the new render phase updates to the previous
Expand All @@ -196,7 +196,12 @@ export function useReducer<S, A>(
// 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;
threepointone marked this conversation as resolved.
Show resolved Hide resolved
} while (update !== null);

Expand All @@ -207,6 +212,7 @@ export function useReducer<S, A>(
}
return [workInProgressHook.memoizedState, dispatch];
} else {
currentlyRenderingComponent = null;
if (reducer === basicStateReducer) {
// Special case for `useState`.
if (typeof initialState === 'function') {
Expand All @@ -215,6 +221,7 @@ export function useReducer<S, A>(
} else if (initialAction !== undefined && initialAction !== null) {
initialState = reducer(initialState, initialAction);
}
currentlyRenderingComponent = cachedCurrentlyRenderingComponent;
workInProgressHook.memoizedState = initialState;
const queue: UpdateQueue<A> = (workInProgressHook.queue = {
last: null,
Expand All @@ -233,7 +240,7 @@ function useMemo<T>(
nextCreate: () => T,
inputs: Array<mixed> | void | null,
): T {
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
let cachedCurrentlyRenderingComponent = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent());
workInProgressHook = createWorkInProgressHook();

const nextInputs =
Expand All @@ -250,7 +257,12 @@ function useMemo<T>(
}
}

threepointone marked this conversation as resolved.
Show resolved Hide resolved
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;
}
Expand Down
20 changes: 16 additions & 4 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ let renderPhaseUpdates: Map<UpdateQueue<any>, Update<any>> | null = null;
// Counter to prevent infinite loops.
let numberOfReRenders: number = 0;
const RE_RENDER_LIMIT = 25;

threepointone marked this conversation as resolved.
Show resolved Hide resolved
threepointone marked this conversation as resolved.
Show resolved Hide resolved
function resolveCurrentlyRenderingFiber(): Fiber {
invariant(
currentlyRenderingFiber !== null,
Expand Down Expand Up @@ -345,7 +344,7 @@ export function useReducer<S, A>(
initialState: S,
initialAction: A | void | null,
): [S, Dispatch<A>] {
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
workInProgressHook = createWorkInProgressHook();
let queue: UpdateQueue<A> | null = (workInProgressHook.queue: any);
if (queue !== null) {
Expand All @@ -366,7 +365,12 @@ export function useReducer<S, A>(
// 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);

Expand Down Expand Up @@ -429,7 +433,9 @@ export function useReducer<S, A>(
} else {
// Process this update.
const action = update.action;
currentlyRenderingFiber = null;
newState = reducer(newState, action);
currentlyRenderingFiber = cachedCurrentlyRenderingFiber;
}
prevUpdate = update;
update = update.next;
Expand All @@ -448,7 +454,7 @@ export function useReducer<S, A>(
const dispatch: Dispatch<A> = (queue.dispatch: any);
return [workInProgressHook.memoizedState, dispatch];
}

threepointone marked this conversation as resolved.
Show resolved Hide resolved
currentlyRenderingFiber = null;
threepointone marked this conversation as resolved.
Show resolved Hide resolved
// There's no existing queue, so this is the initial render.
if (reducer === basicStateReducer) {
// Special case for `useState`.
Expand All @@ -458,6 +464,7 @@ export function useReducer<S, A>(
} else if (initialAction !== undefined && initialAction !== null) {
initialState = reducer(initialState, initialAction);
}
currentlyRenderingFiber = cachedCurrentlyRenderingFiber;
workInProgressHook.memoizedState = workInProgressHook.baseState = initialState;
queue = workInProgressHook.queue = {
last: null,
Expand Down Expand Up @@ -613,7 +620,7 @@ export function useMemo<T>(
nextCreate: () => T,
inputs: Array<mixed> | void | null,
): T {
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe just "fiber" everywhere? It's local so doesn't need too much introducing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't want to encourage reading from it, but I guess that's actually fine

workInProgressHook = createWorkInProgressHook();

const nextInputs =
Expand All @@ -627,7 +634,12 @@ export function useMemo<T>(
}
}

threepointone marked this conversation as resolved.
Show resolved Hide resolved
currentlyRenderingFiber = null;
// now, it'll throw if you try to call a hook inside nextCreate
const nextValue = nextCreate();
// restore the current fiber
threepointone marked this conversation as resolved.
Show resolved Hide resolved
currentlyRenderingFiber = cachedCurrentlyRenderingFiber;

workInProgressHook.memoizedState = [nextValue, nextInputs];
return nextValue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ let ReactFeatureFlags;
let ReactTestRenderer;
let ReactDOMServer;

const OutsideHookScopeError =
'Hooks can only be called inside the body of a function component';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: just copy paste it


// Additional tests can be found in ReactHooksWithNoopRenderer. Plan is to
// gradually migrate those to this file.
describe('ReactHooks', () => {
Expand Down Expand Up @@ -92,4 +95,73 @@ describe('ReactHooks', () => {
const root = ReactTestRenderer.create(<App />);
expect(root.toJSON()).toMatchSnapshot();
});

it("throws when calling hooks inside .memo's compare function", () => {
const {useState} = React;
function App() {
useState(0);
return null;
}
const MemoApp = React.memo(App, () => {
useState(0);
return false;
});

const root = ReactTestRenderer.create(<MemoApp />);
// trying to render again should trigger comparison and throw
expect(() => root.update(<MemoApp />)).toThrow(OutsideHookScopeError);
// the next round, it does a fresh mount, so should render
expect(() => root.update(<MemoApp />)).not.toThrow(OutsideHookScopeError);
// and then again, fail
expect(() => root.update(<MemoApp />)).toThrow(OutsideHookScopeError);
});

it('throws when calling hooks inside useMemo', () => {
const {useMemo, useState} = React;
function App() {
useMemo(() => {
useState(0);
return 123;
});
return null;
}

function Simple() {
useState(0);
return 123;
}
let root = ReactTestRenderer.create(null);
expect(() => root.update(<App />)).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(<Simple />)).not.toThrow();
threepointone marked this conversation as resolved.
Show resolved Hide resolved
});

it('throws when calling hooks inside useReducer', () => {
const {useReducer, useRef} = React;
function App() {
const [value, dispatch] = useReducer((state, action) => {
useRef(0);
return state;
}, 0);
dispatch('foo');
return value;
}
expect(() => ReactTestRenderer.create(<App />)).toThrow(
OutsideHookScopeError,
);
});

it("throws when calling hooks inside useState's initialize function", () => {
const {useState, useRef} = React;
function App() {
useState(() => {
useRef(0);
return 0;
});
}
expect(() => ReactTestRenderer.create(<App />)).toThrow(
OutsideHookScopeError,
);
});
});