-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
fix: don't run effects if a render phase update results in unchanged deps #20676
Changes from 7 commits
c8435e7
5bcce83
6cfd74d
9dfcd56
0f58f40
3cdd353
e39a670
77ed727
5c24bb5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,6 +103,7 @@ import type { | |
ComponentFilter, | ||
ElementType, | ||
} from 'react-devtools-shared/src/types'; | ||
import is from 'shared/objectIs'; | ||
|
||
type getDisplayNameForFiberType = (fiber: Fiber) => string | null; | ||
type getTypeSymbolType = (type: any) => Symbol | number; | ||
|
@@ -1073,6 +1074,48 @@ export function attach( | |
return null; | ||
} | ||
|
||
function areHookInputsEqual( | ||
nextDeps: Array<mixed>, | ||
prevDeps: Array<mixed> | null, | ||
) { | ||
if (prevDeps === null) { | ||
return false; | ||
} | ||
|
||
for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) { | ||
if (is(nextDeps[i], prevDeps[i])) { | ||
continue; | ||
} | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
function isEffect(memoizedState) { | ||
return ( | ||
memoizedState !== null && | ||
typeof memoizedState === 'object' && | ||
memoizedState.hasOwnProperty('tag') && | ||
memoizedState.hasOwnProperty('create') && | ||
memoizedState.hasOwnProperty('destroy') && | ||
memoizedState.hasOwnProperty('deps') && | ||
memoizedState.hasOwnProperty('next') | ||
); | ||
} | ||
|
||
function didHookChange(prev: any, next: any): boolean { | ||
const prevMemoizedState = prev.memoizedState; | ||
const nextMemoizedState = next.memoizedState; | ||
|
||
if (isEffect(prevMemoizedState) && isEffect(nextMemoizedState)) { | ||
return !areHookInputsEqual( | ||
nextMemoizedState.deps, | ||
prevMemoizedState.deps, | ||
); | ||
bvaughn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a fan of this new coupling. Especially since this will not work as expected for e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DevTools probably should be resilient to this happening, however, if you want to kick the can down the road, another solution could be to assign There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you end up doing this, please leave a TODO here so we remember this is accidentally coupled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This was actually my first intuition but I didn't really grasp how But since you seem to be ok with the current approach I'm keeping the initial fix. I added an additional type check to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still planning on looking at this question. It's next in my queue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I thought this might break the Profiler too (how it determines what rendered and what didn't) but I forgot that uses the special |
||
return nextMemoizedState !== prevMemoizedState; | ||
} | ||
|
||
function didHooksChange(prev: any, next: any): boolean { | ||
if (prev == null || next == null) { | ||
return false; | ||
|
@@ -1086,7 +1129,7 @@ export function attach( | |
next.hasOwnProperty('queue') | ||
) { | ||
while (next !== null) { | ||
if (next.memoizedState !== prev.memoizedState) { | ||
if (didHookChange(prev, next)) { | ||
return true; | ||
} else { | ||
next = next.next; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1328,7 +1328,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void { | |||||||||||||||||||
if (nextDeps !== null) { | ||||||||||||||||||||
const prevDeps = prevEffect.deps; | ||||||||||||||||||||
if (areHookInputsEqual(nextDeps, prevDeps)) { | ||||||||||||||||||||
pushEffect(hookFlags, create, destroy, nextDeps); | ||||||||||||||||||||
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps); | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem like the correct approach. I would rather restore the memoized state when we trigger a render phase update. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried - pushEffect(hookFlags, create, destroy, nextDeps);
+ const effect = pushEffect(hookFlags, create, destroy, nextDeps);
+ if (!areHookInputsEqual(effect.deps, hook.memoizedState.deps)) {
+ hook.memoizedState = effect;
+ } locally which makes more sense conceptually. However, this results in a duplicate warning in react/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js Lines 614 to 622 in d17086c
- pushEffect(hookFlags, create, destroy, nextDeps);
+ const effect = pushEffect(hookFlags, create, destroy, nextDeps);
+ if (
+ didScheduleRenderPhaseUpdate &&
+ !areHookInputsEqual(effect.deps, hook.memoizedState.deps)
+ ) {
+ hook.memoizedState = effect;
+ } passes all tests locally but seems even more sketchy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix you committed is more correct. We should compare against the current hook, not the hook from the pass where the render phase update happened. The bug is that in the case of a render phase update, we're not persisting the most current hook. We're persisting the "intermediate" hook that happens before the render phase update replaces it. Is there a reason you don't like your fix that I'm overlooking? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The failed devtools test prompted my response. Right now we're invalidating the memoizedState even though the deps didn't change. I suspected that this may cause performance regressions in other places considering that everywhere we compare But maybe memoizedState was never intended to be used that way. It might make sense to compare the actual deps not just memoizedState in devtools. I'll look into the test now. Thanks for the clarification 👍 |
||||||||||||||||||||
return; | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would the return values for
isEffect(prevMemoizedState)
andisEffect(nextMemoizedState)
ever differ? Can we just checkisEffect(nextMemoizedState)
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. But I haven't checked if Effect hooks start with
memoizedState = null
. I'll see if can improve the type story here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they did, that wouldn't matter here because we early return in
didHooksChange
if either one was null.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just do this tweak in a follow up? :) Would be nice.