Throw an error when using hooks inside useMemo, or .memo's comparison function #14608
Conversation
|
ReactDOM: size: 0.0%, gzip: 0.0% Details of bundled changes.Comparing: be457ca...3242f68 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
| @@ -342,13 +347,16 @@ function updateMemoComponent( | |||
| // Default to shallow comparison | |||
| let compare = Component.compare; | |||
| compare = compare !== null ? compare : shallowEqual; | |||
| pauseHooks(true); | |||
There was a problem hiding this comment.
I think the test passes without these calls. Memo doesn't call "prepare to use hooks" so we're not in an area where there's a current fiber. You should be able to just remove them.
There was a problem hiding this comment.
oh interesting, you're right they pass. I recollect how I broke this - I had a bad test, and 'fixed' it after I had my code in place. bleurgh, thanks for the catch.
| @@ -626,8 +633,9 @@ export function useMemo<T>( | |||
| return prevState[0]; | |||
| } | |||
| } | |||
|
|
|||
| pauseHooks(true) | |||
There was a problem hiding this comment.
Since we don't need pauseHooks elsewhere this can become local isInUseMemo = true.
There was a problem hiding this comment.
agreed, that was my first cut as well.
|
What about |
|
Instead of adding a second check, let's just null out |
the function itself isn't called during render, no? so I assume it should throw, but I'll add a test to verify. |
I'm happy to do this. either approach is similarly annoying tbh ha. |
I don't think they're exactly equivalent. This is a very hot path. Having one variable and doing just one check per Hook seems better than doing two. With this solution, only The downside is that it's an extra local variable (to hold the current Fiber) and Sebastian says this can go beyond the number of registers and cause a perf cliff. I don't know about this. I'd probably go with one extra variable though for less common hooks than an extra check for every hook.
Ahh I see. I meant this is a valid code: const renderRow = useCallback((item) => {
return <Row item={item} theme={theme} />
}, [theme])
<Table renderRow={renderRow} />
// in Table
rows.map(props.renderRow)So callback itself can execute during render phase and it could be nice if we could disallow calling Hooks in it. (They would be attached to the Table which is super confusing.) Reading context there is probably also bad because it would read Table's context rather than Row's. But you're right we're not actually proxying the function so we can't add extra logic to it. Theoretically we could in DEV though. But we can talk about this another time. |
Good points. I'll make the changes, and keep an eye on it. |
|
Curious, have you ever got to a situation where comparisons are that expensive? |
|
In React everything that executes every render is expensive. It's the hottest path in the application. |
|
(In isolation, no, but these things add up and at some point you end up with too many — not knowing which ones are safe to remove. So we try to not add them whenever possible.) |
|
I made changes. I avoided the 'local' var check with yet another module-local var. I'll add tests for the server renderer now. |
| @@ -229,11 +229,14 @@ export function useReducer<S, A>( | |||
| } | |||
| } | |||
|
|
|||
| // a variable to store current component while we compare memo values | |||
| let cachedCurrentlyRenderingComponent = null; | |||
There was a problem hiding this comment.
Why is this top level rather than inside useMemo?
There was a problem hiding this comment.
it's one less var to be gced, reuses the location across runs. you mentioned the local var has a cost I assumed you'd want to avoid?
There was a problem hiding this comment.
maybe I should just trust javascript engines
There was a problem hiding this comment.
Declaring variables pointing to shared objects has no effect on GC — I assume you mean the stack? I said it's a hypothetical concern but I'm not actually sure it matters and I think local function variable is fine.
There was a problem hiding this comment.
I wish we had some easy way to validate these JS engine assumptions, it’d be interesting to see how often they’re true 🤔
There was a problem hiding this comment.
Well, having two checks is likely going to be more expensive than having one check, no matter what you do.
There was a problem hiding this comment.
Yeah let's keep this local to useMemo.
| @@ -616,11 +608,14 @@ export function useCallback<T>( | |||
| return callback; | |||
| } | |||
|
|
|||
| // a variable to store current component while we compare memo values | |||
| let cachedCurrentlyRenderingFiber = null; | |||
| @@ -633,9 +628,14 @@ export function useMemo<T>( | |||
| return prevState[0]; | |||
| } | |||
| } | |||
| pauseHooks(true) | |||
|
|
|||
| // null the reference to the component | |||
There was a problem hiding this comment.
This comment is unnecessary, the line below says the same thing.
There was a problem hiding this comment.
Good start. Aside from the nits, there are some missing pieces (not sure if you're intending to do these in a follow-up):
readContext(the one on the dispatcher) is not a hook but it also needs to be prevented inside these functions- Forbid Hook and context access inside these additional render phase functions:
- Reducers (which includes setState update functions)
- The State Hook's lazy state initializer
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
Outdated
Show resolved
Hide resolved
| @@ -229,11 +229,14 @@ export function useReducer<S, A>( | |||
| } | |||
| } | |||
|
|
|||
| // a variable to store current component while we compare memo values | |||
| let cachedCurrentlyRenderingComponent = null; | |||
There was a problem hiding this comment.
Yeah let's keep this local to useMemo.
|
I've updated the PR addressing most of the comments, including adding tests for the server side stuff. one last bit is to prevent |
| export function useMemo<T>( | ||
| nextCreate: () => T, | ||
| inputs: Array<mixed> | void | null, | ||
| ): T { | ||
| cachedCurrentlyRenderingFiber = currentlyRenderingFiber = resolveCurrentlyRenderingFiber(); | ||
| let cachedCurrentlyRenderingFiber = (currentlyRenderingFiber = resolveCurrentlyRenderingFiber()); |
There was a problem hiding this comment.
Nit: Maybe just "fiber" everywhere? It's local so doesn't need too much introducing.
There was a problem hiding this comment.
didn't want to encourage reading from it, but I guess that's actually fine
| @@ -17,6 +17,9 @@ let ReactFeatureFlags; | |||
| let ReactTestRenderer; | |||
| let ReactDOMServer; | |||
|
|
|||
| const OutsideHookScopeError = | |||
| 'Hooks can only be called inside the body of a function component'; | |||
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
Outdated
Show resolved
Hide resolved
|
alright, I'm happy with this, will land after ci finishes. thanks everyone! |
|
@acdlite do you want to have a onceover, or should I dismiss your review? plsthx |
…react into hooks-inside-memo-compare
|
I'll send a separate PR for guarding eagerReducer, I want to add a fresh test for that. |
|
#fileatask ;( |
…r .memo's comparator (facebook#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
…r .memo's comparator (facebook#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
This PR adds a flag to 'pause' hooks while they're running, so we can throw an error whenever a hook is called inside 2 spots - useMemo's nextCreate function, and React.memo's comparison function.
It's a bit icky, but wanted to confirm the approach with you.
I could also rename pauseHooks to something more explicit, like pauseHooksWhileComparingMemos