Skip to content

Commit

Permalink
Throw an error when using hooks inside useMemo/useState/useReducer, o…
Browse files Browse the repository at this point in the history
…r .memo's comparator (#14608)

* hooks inside useMemo/.memo - failing tests

* throw an error when using hooks inside useMemo

* throw when using hooks inside .memo's compare fn

* faster/better/stronger

* same logic for useReducer, tests for the server, etc

* Update ReactDOMServerIntegrationHooks-test.internal.js

ack lint

* nits

* whitespace

* whitespace

* stray semi

* Tweak comment

* stray unmatched fiber reset

* nit
  • Loading branch information
Sunil Pai authored Jan 18, 2019
1 parent be457ca commit e1cd83e
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,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
12 changes: 10 additions & 2 deletions packages/react-dom/src/server/ReactPartialRendererHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ export function useReducer<S, A>(
currentHookNameInDev = 'useReducer';
}
}
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
let component = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent());
workInProgressHook = createWorkInProgressHook();
if (isReRender) {
// This is a re-render. Apply the new render phase updates to the previous
Expand All @@ -253,7 +253,10 @@ export function useReducer<S, A>(
// 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;
newState = reducer(newState, action);
currentlyRenderingComponent = component;
update = update.next;
} while (update !== null);

Expand All @@ -264,6 +267,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 @@ -272,6 +276,7 @@ export function useReducer<S, A>(
} else if (initialAction !== undefined && initialAction !== null) {
initialState = reducer(initialState, initialAction);
}
currentlyRenderingComponent = component;
workInProgressHook.memoizedState = initialState;
const queue: UpdateQueue<A> = (workInProgressHook.queue = {
last: null,
Expand All @@ -287,7 +292,7 @@ export function useReducer<S, A>(
}

function useMemo<T>(nextCreate: () => T, deps: Array<mixed> | void | null): T {
currentlyRenderingComponent = resolveCurrentlyRenderingComponent();
let component = (currentlyRenderingComponent = resolveCurrentlyRenderingComponent());
workInProgressHook = createWorkInProgressHook();

const nextDeps = deps === undefined ? null : deps;
Expand All @@ -304,7 +309,10 @@ function useMemo<T>(nextCreate: () => T, deps: Array<mixed> | void | null): T {
}
}

// Temporarily clear to forbid calling Hooks.
currentlyRenderingComponent = null;
const nextValue = nextCreate();
currentlyRenderingComponent = component;
workInProgressHook.memoizedState = [nextValue, nextDeps];
return nextValue;
}
Expand Down
17 changes: 14 additions & 3 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,7 +420,7 @@ export function useReducer<S, A>(
currentHookNameInDev = 'useReducer';
}
}
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
workInProgressHook = createWorkInProgressHook();
let queue: UpdateQueue<S, A> | null = (workInProgressHook.queue: any);
if (queue !== null) {
Expand All @@ -441,7 +441,10 @@ export function useReducer<S, A>(
// priority because it will always be the same as the current
// render's.
const action = update.action;
// Temporarily clear to forbid calling Hooks in a reducer.
currentlyRenderingFiber = null;
newState = reducer(newState, action);
currentlyRenderingFiber = fiber;
update = update.next;
} while (update !== null);

Expand Down Expand Up @@ -510,7 +513,10 @@ export function useReducer<S, A>(
newState = ((update.eagerState: any): S);
} else {
const action = update.action;
// Temporarily clear to forbid calling Hooks in a reducer.
currentlyRenderingFiber = null;
newState = reducer(newState, action);
currentlyRenderingFiber = fiber;
}
}
prevUpdate = update;
Expand Down Expand Up @@ -539,7 +545,8 @@ export function useReducer<S, A>(
const dispatch: Dispatch<A> = (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) {
// Special case for `useState`.
Expand All @@ -549,6 +556,7 @@ export function useReducer<S, A>(
} else if (initialAction !== undefined && initialAction !== null) {
initialState = reducer(initialState, initialAction);
}
currentlyRenderingFiber = fiber;
workInProgressHook.memoizedState = workInProgressHook.baseState = initialState;
queue = workInProgressHook.queue = {
last: null,
Expand Down Expand Up @@ -739,7 +747,7 @@ export function useMemo<T>(
if (__DEV__) {
currentHookNameInDev = 'useMemo';
}
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
let fiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber());
workInProgressHook = createWorkInProgressHook();

const nextDeps = deps === undefined ? null : deps;
Expand All @@ -755,7 +763,10 @@ export function useMemo<T>(
}
}

// Temporarily clear to forbid calling Hooks.
currentlyRenderingFiber = null;
const nextValue = nextCreate();
currentlyRenderingFiber = fiber;
workInProgressHook.memoizedState = [nextValue, nextDeps];
return nextValue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,4 +517,83 @@ 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(
'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(<MemoApp />)).not.toThrow(
'Hooks can only be called inside the body of a function component',
);
// and then again, fail
expect(() => root.update(<MemoApp />)).toThrow(
'Hooks can only be called inside the body of a function component',
);
});

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

function Simple() {
const [value] = useState(123);
return value;
}
let root = ReactTestRenderer.create(null);
expect(() => root.update(<App />)).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(<Simple />);
expect(root.toJSON()).toEqual('123');
});

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(
'Hooks can only be called inside the body of a function component',
);
});

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(
'Hooks can only be called inside the body of a function component',
);
});
});

0 comments on commit e1cd83e

Please sign in to comment.