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

[eps1lon]

Summary

Fixes #20675

The memoized state of effect hooks is only invalidated when deps change. Deps are compared between the previous effect and the current effect. This can be problematic if one commit consists of an update that has changed deps followed by an update that has equal deps. That commit will lead to memoizedState containing the changed deps even though we committed with unchanged deps.

The n+1 update will therefore run an effect because we compare the updated deps with the deps with which we never actually committed.

To prevent this we now invalidate memoizedState on every updateEffectImpl call so that memoizedStat.deps always points to the latest deps.

Test Plan

• CI green
• codesandbox of Bug: FunctionComponent re-render phase cause a bug #20675 has expected behavior
  All codesandboxes in fix: don't run effects if a render phase update results in unchanged deps #20676 (comment) have the expected behavior (no effects are logged after clicking)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5c24bb5:

Sandbox	Source
React	Configuration
ReactHooks	Issue #20675
ReactHooks	Issue #20675
React	Issue #20675

[sizebot]

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 5c24bb5

[sizebot]

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 5c24bb5

[eps1lon]

This doesn't seem like the correct approach. I would rather restore the memoized state when we trigger a render phase update.

[eps1lon]

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.

[acdlite]

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?

[eps1lon]

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 👍

[acdlite]

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

[acdlite]

Looks like there's a DevTools test failure. I'm guessing it's a legit change, so the snapshot just needs to be updated. Have you looked into that yet?

[eps1lon]

Not a fan of this new coupling. Especially since this will not work as expected for e.g. useState({ tag, create, destroy, deps, next }); even though that's unlikely to occur in userland. @bvaughn Any suggestions how this should work now that hook.memoizedState always changes even if deps didn't?

[acdlite]

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 hook.memoizedState = currentHook.memoizedState when you bailout. Instead of the result of pushEffect. In the case of of a normal update, it's a no-op because that's what it already points to. But for render phase updates, it will restore the original object.

[acdlite]

If you end up doing this, please leave a TODO here so we remember this is accidentally coupled

[eps1lon]

But for render phase updates, it will restore the original object.

This was actually my first intuition but I didn't really grasp how hook relates to currentHook.

But since you seem to be ok with the current approach I'm keeping the initial fix. I added an additional type check to memoizedState.deps in case someone has useState({ deps: 1, tag, create, destroy, next });

[bvaughn]

Still planning on looking at this question. It's next in my queue.

[bvaughn]

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

[acdlite]

Great test case, great sandbox, great fix 👏 Thank you!

(Let's have @bvaughn sign off on the DevTools changes before merge, though.)

[bvaughn]

When would the return values for isEffect(prevMemoizedState) and isEffect(nextMemoizedState) ever differ? Can we just check isEffect(nextMemoizedState) here?

[eps1lon]

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.

[bvaughn]

If they did, that wouldn't matter here because we early return in didHooksChange if either one was null.

[bvaughn]

Let's just do this tweak in a follow up? :) Would be nice.

[bvaughn]

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

[bvaughn]

Thanks @eps1lon !