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.

Sign up for GitHub

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

Jump to bottom

fix: don't run effects if a render phase update results in unchanged deps #20676

Merged
merged 9 commits into from
Jan 29, 2021
Merged

fix: don't run effects if a render phase update results in unchanged deps #20676

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c8435e7
current behavior of effect dependencies on render phase updates
eps1lon Jan 27, 2021
5bcce83
fix: don't schedule effects when render phase updates aren't committed
eps1lon Jan 27, 2021
6cfd74d
apply fix to new fork
eps1lon Jan 27, 2021
9dfcd56
Use suggested test name
eps1lon Jan 27, 2021
0f58f40
devtools: compare effects by deps not referential equality
eps1lon Jan 27, 2021
3cdd353
disable type checking for added functions
eps1lon Jan 27, 2021
e39a670
bump CircleCI
eps1lon Jan 27, 2021
77ed727
Be more defensive about `useState({ deps })`
eps1lon Jan 28, 2021
5c24bb5
bail out early if memoizedState didn't change
eps1lon Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator Author

@eps1lon eps1lon Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

expect(() => {
root.update(<App dependencies={['A', 'B']} />);
}).toErrorDev([
'Warning: The final argument passed to useLayoutEffect changed size ' +
'between renders. The order and size of this array must remain ' +
'constant.\n\n' +
'Previous: [A]\n' +
'Incoming: [A, B]\n',
]); 

-        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.

Copy link
Collaborator

@acdlite acdlite Jan 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a reason you don't like your fix that I'm overlooking?

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 hook.memoizedState === prevHook.memoizedState it'll now evaluate to false even though the deps didn't change.

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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,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);
return;
}
}
Expand Down
43 changes: 43 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3760,4 +3760,47 @@ describe('ReactHooksWithNoopRenderer', () => {
// effects would not fire.
expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']);
});

it('effect dependencies are consistent with yielded values when triggering state updates during render', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested alt name for test: "effect dependencies are persisted after a render phase update"

let handleClick;
function Test() {
const [count, setCount] = useState(0);

useEffect(() => {
Scheduler.unstable_yieldValue(`Effect: ${count}`);
}, [count]);

if (count > 0) {
setCount(0);
}

handleClick = () => setCount(2);

return <Text text={`Render: ${count}`} />;
}

ReactNoop.act(() => {
ReactNoop.render(<Test />);
});

expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']);

ReactNoop.act(() => {
handleClick();
});

expect(Scheduler).toHaveYielded(['Render: 0']);

ReactNoop.act(() => {
handleClick();
});

expect(Scheduler).toHaveYielded(['Render: 0']);

ReactNoop.act(() => {
handleClick();
});

expect(Scheduler).toHaveYielded(['Render: 0']);
});
});