[crud] Fix deps comparison bug (#31599)

Fixes a bug with the experimental `useResourceEffect` hook where we would compare the wrong deps when there happened to be another kind of effect preceding the ResourceEffect. To do this correctly we need to add a pointer to the ResourceEffect's identity on the update. I also unified the previously separate push effect impls for resource effects since they are always pushed together as a unit.
2026-01-15 12:15:22 +00:00 · 2024-11-20 16:54:41 -05:00
parent 64f89510af
commit c11c9510fa
3 changed files with 123 additions and 50 deletions
--- a/packages/react-reconciler/src/ReactFiberHooks.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -253,6 +253,7 @@ export type ResourceEffectUpdate = {
  update: ((resource: mixed) => void) | void,
  deps: Array<mixed> | void | null,
  next: Effect,
+  identity: ResourceEffectIdentity,
 };

 type StoreInstance<T> = {
@@ -2585,40 +2586,37 @@ function pushSimpleEffect(
  return pushEffectImpl(effect);
 }

-function pushResourceEffectIdentity(
-  tag: HookFlags,
+function pushResourceEffect(
+  identityTag: HookFlags,
+  updateTag: HookFlags,
  inst: EffectInstance,
  create: () => mixed,
-  deps: Array<mixed> | void | null,
-): Effect {
-  const effect: ResourceEffectIdentity = {
-    resourceKind: ResourceEffectIdentityKind,
-    tag,
-    create,
-    deps,
-    inst,
-    // Circular
-    next: (null: any),
-  };
-  return pushEffectImpl(effect);
-}
-
-function pushResourceEffectUpdate(
-  tag: HookFlags,
-  inst: EffectInstance,
+  createDeps: Array<mixed> | void | null,
  update: ((resource: mixed) => void) | void,
-  deps: Array<mixed> | void | null,
+  updateDeps: Array<mixed> | void | null,
 ): Effect {
-  const effect: ResourceEffectUpdate = {
-    resourceKind: ResourceEffectUpdateKind,
-    tag,
-    update,
-    deps,
+  const effectIdentity: ResourceEffectIdentity = {
+    resourceKind: ResourceEffectIdentityKind,
+    tag: identityTag,
+    create,
+    deps: createDeps,
    inst,
    // Circular
    next: (null: any),
  };
-  return pushEffectImpl(effect);
+  pushEffectImpl(effectIdentity);
+
+  const effectUpdate: ResourceEffectUpdate = {
+    resourceKind: ResourceEffectUpdateKind,
+    tag: updateTag,
+    update,
+    deps: updateDeps,
+    inst,
+    identity: effectIdentity,
+    // Circular
+    next: (null: any),
+  };
+  return pushEffectImpl(effectUpdate);
 }

 function pushEffectImpl(effect: Effect): Effect {
@@ -2792,15 +2790,12 @@ function mountResourceEffectImpl(
  currentlyRenderingFiber.flags |= fiberFlags;
  const inst = createEffectInstance();
  inst.destroy = destroy;
-  hook.memoizedState = pushResourceEffectIdentity(
+  hook.memoizedState = pushResourceEffect(
    HookHasEffect | hookFlags,
+    hookFlags,
    inst,
    create,
    createDeps,
-  );
-  hook.memoizedState = pushResourceEffectUpdate(
-    hookFlags,
-    inst,
    update,
    updateDeps,
  );
@@ -2847,25 +2842,31 @@ function updateResourceEffectImpl(
    const prevEffect: Effect = currentHook.memoizedState;
    if (nextCreateDeps !== null) {
      let prevCreateDeps;
-      // Seems sketchy but in practice we always push an Identity and an Update together. For safety
-      // we error in DEV if this does not hold true.
-      if (prevEffect.resourceKind === ResourceEffectUpdateKind) {
+      if (
+        prevEffect.resourceKind != null &&
+        prevEffect.resourceKind === ResourceEffectUpdateKind
+      ) {
        prevCreateDeps =
-          prevEffect.next.deps != null ? prevEffect.next.deps : null;
+          prevEffect.identity.deps != null ? prevEffect.identity.deps : null;
      } else {
-        if (__DEV__) {
-          console.error(
-            'Expected a ResourceEffectUpdateKind to be pushed together with ' +
-              'ResourceEffectIdentityKind, got %s. This is a bug in React.',
-            prevEffect.resourceKind,
-          );
-        }
-        prevCreateDeps = prevEffect.deps != null ? prevEffect.deps : null;
+        throw new Error(
+          `Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
+        );
      }
      isCreateDepsSame = areHookInputsEqual(nextCreateDeps, prevCreateDeps);
    }
    if (nextUpdateDeps !== null) {
-      const prevUpdateDeps = prevEffect.deps != null ? prevEffect.deps : null;
+      let prevUpdateDeps;
+      if (
+        prevEffect.resourceKind != null &&
+        prevEffect.resourceKind === ResourceEffectUpdateKind
+      ) {
+        prevUpdateDeps = prevEffect.deps != null ? prevEffect.deps : null;
+      } else {
+        throw new Error(
+          `Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React.`,
+        );
+      }
      isUpdateDepsSame = areHookInputsEqual(nextUpdateDeps, prevUpdateDeps);
    }
  }
@@ -2874,15 +2875,12 @@ function updateResourceEffectImpl(
    currentlyRenderingFiber.flags |= fiberFlags;
  }

-  hook.memoizedState = pushResourceEffectIdentity(
+  hook.memoizedState = pushResourceEffect(
    isCreateDepsSame ? hookFlags : HookHasEffect | hookFlags,
+    isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
    inst,
    create,
    nextCreateDeps,
-  );
-  hook.memoizedState = pushResourceEffectUpdate(
-    isUpdateDepsSame ? hookFlags : HookHasEffect | hookFlags,
-    inst,
    update,
    nextUpdateDeps,
  );
--- a/packages/react-reconciler/src/tests/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/tests/ReactHooksWithNoopRenderer-test.js
@@ -3927,6 +3927,79 @@ describe('ReactHooksWithNoopRenderer', () => {
        </>,
      );
    });
+
+    // @gate enableUseResourceEffectHook
+    it('composes with other kinds of effects', async () => {
+      let rerender;
+      function App({id, username}) {
+        const [count, rerender_] = useState(0);
+        rerender = rerender_;
+        const opts = useMemo(() => {
+          return {username};
+        }, [username]);
+        useEffect(() => {
+          Scheduler.log(`useEffect(${count})`);
+        }, [count]);
+        useResourceEffect(
+          () => {
+            const resource = new Resource(id, opts);
+            Scheduler.log(`create(${resource.id}, ${resource.opts.username})`);
+            return resource;
+          },
+          [id],
+          resource => {
+            resource.update(opts);
+            Scheduler.log(`update(${resource.id}, ${resource.opts.username})`);
+          },
+          [opts],
+          resource => {
+            resource.destroy();
+            Scheduler.log(`destroy(${resource.id}, ${resource.opts.username})`);
+          },
+        );
+        return null;
+      }
+
+      await act(() => {
+        ReactNoop.render(<App id={1} username="Jack" />);
+      });
+      assertLog(['useEffect(0)', 'create(1, Jack)']);
+
+      await act(() => {
+        ReactNoop.render(<App id={1} username="Lauren" />);
+      });
+      assertLog(['update(1, Lauren)']);
+
+      await act(() => {
+        ReactNoop.render(<App id={1} username="Lauren" />);
+      });
+      assertLog([]);
+
+      await act(() => {
+        ReactNoop.render(<App id={1} username="Jordan" />);
+      });
+      assertLog(['update(1, Jordan)']);
+
+      await act(() => {
+        rerender(n => n + 1);
+      });
+      assertLog(['useEffect(1)']);
+
+      await act(() => {
+        ReactNoop.render(<App id={1} username="Mofei" />);
+      });
+      assertLog(['update(1, Mofei)']);
+
+      await act(() => {
+        ReactNoop.render(<App id={2} username="Jack" />);
+      });
+      assertLog(['destroy(1, Mofei)', 'create(2, Jack)']);
+
+      await act(() => {
+        ReactNoop.render(null);
+      });
+      assertLog(['destroy(2, Jack)']);
+    });
  });

  describe('useCallback', () => {
--- a/scripts/error-codes/codes.json
+++ b/scripts/error-codes/codes.json
@@ -527,5 +527,7 @@
  "539": "Binary RSC chunks cannot be encoded as strings. This is a bug in the wiring of the React streams.",
  "540": "String chunks need to be passed in their original shape. Not split into smaller string chunks. This is a bug in the wiring of the React streams.",
  "541": "Compared context values must be arrays",
-  "542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary."
+  "542": "Suspense Exception: This is not a real error! It's an implementation detail of `useActionState` to interrupt the current render. You must either rethrow it immediately, or move the `useActionState` call outside of the `try/catch` block. Capturing without rethrowing will lead to unexpected behavior.\n\nTo handle async errors, wrap your component in an error boundary.",
+  "543": "Expected a ResourceEffectUpdate to be pushed together with ResourceEffectIdentity. This is a bug in React."
 }
+