[crud] Fix deps comparison bug (#31599)

poteto · web-flow · commit c11c9510fa14 · 2024-11-20T16:54:41.000-05:00
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.
diff --git 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,
+  createDeps: Array<mixed> | void | null,
+  update: ((resource: mixed) => void) | void,
+  updateDeps: Array<mixed> | void | null,
 ): Effect {
-  const effect: ResourceEffectIdentity = {
+  const effectIdentity: ResourceEffectIdentity = {
     resourceKind: ResourceEffectIdentityKind,
-    tag,
+    tag: identityTag,
     create,
-    deps,
+    deps: createDeps,
     inst,
     // Circular
     next: (null: any),
   };
-  return pushEffectImpl(effect);
-}
+  pushEffectImpl(effectIdentity);
 
-function pushResourceEffectUpdate(
-  tag: HookFlags,
-  inst: EffectInstance,
-  update: ((resource: mixed) => void) | void,
-  deps: Array<mixed> | void | null,
-): Effect {
-  const effect: ResourceEffectUpdate = {
+  const effectUpdate: ResourceEffectUpdate = {
     resourceKind: ResourceEffectUpdateKind,
-    tag,
+    tag: updateTag,
     update,
-    deps,
+    deps: updateDeps,
     inst,
+    identity: effectIdentity,
     // Circular
     next: (null: any),
   };
-  return pushEffectImpl(effect);
+  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,
   );
diff --git 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', () => {
diff --git 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."
 }
+
