[crud] Merge useResourceEffect into useEffect

Merges the useResourceEffect API into useEffect while keeping the underlying implementation the same. useResourceEffect will be removed in the next diff. To fork between behavior we rely on a `typeof` check for the updater or destroy function in addition to the CRUD feature flag. This does now have to be checked every time (instead of inlined statically like before due to them being different hooks) which will incur some non-zero amount (possibly negligble) of overhead for every effect.
facebook · Jan 30, 2025 · 601b4ea · 601b4ea
1 parent 7f3a959
commit 601b4ea
Show file tree

Hide file tree

Showing 9 changed files with 279 additions and 143 deletions.
diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js
@@ -373,8 +373,11 @@ function useInsertionEffect(
 }
 
 function useEffect(
-  create: () => (() => void) | void,
-  inputs: Array<mixed> | void | null,
+  create: (() => (() => void) | void) | (() => {...} | void | null),
+  createDeps: Array<mixed> | void | null,
+  update?: ((resource: {...} | void | null) => void) | void,
+  updateDeps?: Array<mixed> | void | null,
+  destroy?: ((resource: {...} | void | null) => void) | void,
 ): void {
   nextHook();
   hookLog.push({

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
@@ -27,7 +27,6 @@ let useRef;
 let useImperativeHandle;
 let useInsertionEffect;
 let useLayoutEffect;
-let useResourceEffect;
 let useDebugValue;
 let forwardRef;
 let yieldedValues;
@@ -52,7 +51,6 @@ function initModules() {
   useImperativeHandle = React.useImperativeHandle;
   useInsertionEffect = React.useInsertionEffect;
   useLayoutEffect = React.useLayoutEffect;
-  useResourceEffect = React.experimental_useResourceEffect;
   forwardRef = React.forwardRef;
 
   yieldedValues = [];
@@ -655,15 +653,15 @@ describe('ReactDOMServerHooks', () => {
     });
   });
 
-  describe('useResourceEffect', () => {
+  describe('useEffect with CRUD overload', () => {
     gate(flags => {
       if (flags.enableUseEffectCRUDOverload) {
         const yields = [];
         itRenders(
           'should ignore resource effects on the server',
           async render => {
             function Counter(props) {
-              useResourceEffect(
+              useEffect(
                 () => {
                   yieldValue('created on client');
                   return {resource_counter: props.count};

diff --git a/packages/react-reconciler/src/ReactFiberCallUserSpace.js b/packages/react-reconciler/src/ReactFiberCallUserSpace.js
@@ -254,7 +254,7 @@ const callDestroy = {
 export const callDestroyInDEV: (
   current: Fiber,
   nearestMountedAncestor: Fiber | null,
-  destroy: () => void,
+  destroy: (() => void) | (({...}) => void),
 ) => void = __DEV__
   ? // We use this technique to trick minifiers to preserve the function name.
     (callDestroy['react-stack-bottom-frame'].bind(callDestroy): any)

diff --git a/packages/react-reconciler/src/ReactFiberCommitEffects.js b/packages/react-reconciler/src/ReactFiberCommitEffects.js
@@ -170,8 +170,9 @@ export function commitHookEffectListMount(
                 );
                 if (effect.inst.resource == null) {
                   console.error(
-                    'useResourceEffect must provide a callback which returns a resource. ' +
-                      'If a managed resource is not needed here, use useEffect. Received %s',
+                    'useEffect must provide a callback which returns a resource. ' +
+                      'If a managed resource is not needed here, do not provide an updater or ' +
+                      'destroy callback. Received %s',
                     effect.inst.resource,
                   );
                 }
@@ -261,11 +262,6 @@ export function commitHookEffectListMount(
                 hookName = 'useLayoutEffect';
               } else if ((effect.tag & HookInsertion) !== NoFlags) {
                 hookName = 'useInsertionEffect';
-              } else if (
-                enableUseEffectCRUDOverload &&
-                effect.resourceKind != null
-              ) {
-                hookName = 'useResourceEffect';
               } else {
                 hookName = 'useEffect';
               }
@@ -363,7 +359,7 @@ export function commitHookEffectListUnmount(
                 effect.resourceKind === ResourceEffectIdentityKind &&
                 effect.inst.resource != null
               ) {
-                safelyCallDestroyWithResource(
+                safelyCallDestroy(
                   finishedWork,
                   nearestMountedAncestor,
                   destroy,
@@ -1015,32 +1011,11 @@ export function safelyDetachRef(
 function safelyCallDestroy(
   current: Fiber,
   nearestMountedAncestor: Fiber | null,
-  destroy: () => void,
-) {
-  if (__DEV__) {
-    runWithFiberInDEV(
-      current,
-      callDestroyInDEV,
-      current,
-      nearestMountedAncestor,
-      destroy,
-    );
-  } else {
-    try {
-      destroy();
-    } catch (error) {
-      captureCommitPhaseError(current, nearestMountedAncestor, error);
-    }
-  }
-}
-
-function safelyCallDestroyWithResource(
-  current: Fiber,
-  nearestMountedAncestor: Fiber | null,
-  destroy: ({...}) => void,
-  resource: {...},
+  destroy: (() => void) | (({...}) => void),
+  resource?: {...} | void | null,
 ) {
-  const destroy_ = destroy.bind(null, resource);
+  // $FlowFixMe[extra-arg] @poteto this is safe either way because the extra arg is ignored if it's not a CRUD effect
+  const destroy_ = resource == null ? destroy : destroy.bind(null, resource);
   if (__DEV__) {
     runWithFiberInDEV(
       current,