[Experiment] Warn if callback ref returns a function

facebook · Sep 14, 2021 · 725bad1 · 725bad1
1 parent 50263d3
commit 725bad1
Show file tree

Hide file tree

Showing 13 changed files with 103 additions and 8 deletions.
diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js
@@ -351,6 +351,36 @@ describe('ref swapping', () => {
       'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
     );
   });
+
+  // @gate warnAboutCallbackRefReturningFunction
+  it('should warn about callback refs returning a function', () => {
+    const container = document.createElement('div');
+    expect(() => {
+      ReactDOM.render(<div ref={() => () => {}} />, container);
+    }).toErrorDev('Unexpected return value from a callback ref in div');
+
+    // Cleanup should warn, too.
+    expect(() => {
+      ReactDOM.render(<span />, container);
+    }).toErrorDev('Unexpected return value from a callback ref in div', {
+      withoutStack: true,
+    });
+
+    // No warning when returning non-functions.
+    ReactDOM.render(<p ref={() => ({})} />, container);
+    ReactDOM.render(<p ref={() => null} />, container);
+    ReactDOM.render(<p ref={() => undefined} />, container);
+
+    // Still warns on functions (not deduped).
+    expect(() => {
+      ReactDOM.render(<div ref={() => () => {}} />, container);
+    }).toErrorDev('Unexpected return value from a callback ref in div');
+    expect(() => {
+      ReactDOM.unmountComponentAtNode(container);
+    }).toErrorDev('Unexpected return value from a callback ref in div', {
+      withoutStack: true,
+    });
+  });
 });
 
 describe('root level refs', () => {

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -37,6 +37,7 @@ import {
   deletedTreeCleanUpLevel,
   enableSuspenseLayoutEffectSemantics,
   enableUpdaterTracking,
+  warnAboutCallbackRefReturningFunction,
 } from 'shared/ReactFeatureFlags';
 import {
   FunctionComponent,
@@ -250,6 +251,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
   const ref = current.ref;
   if (ref !== null) {
     if (typeof ref === 'function') {
+      let retVal;
       try {
         if (
           enableProfilerTimer &&
@@ -258,17 +260,29 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
         ) {
           try {
             startLayoutEffectTimer();
-            ref(null);
+            retVal = ref(null);
           } finally {
             recordLayoutEffectDuration(current);
           }
         } else {
-          ref(null);
+          retVal = ref(null);
         }
       } catch (error) {
         reportUncaughtErrorInDEV(error);
         captureCommitPhaseError(current, nearestMountedAncestor, error);
       }
+      if (__DEV__) {
+        if (
+          warnAboutCallbackRefReturningFunction &&
+          typeof retVal === 'function'
+        ) {
+          console.error(
+            'Unexpected return value from a callback ref in %s. ' +
+              'A callback ref should not return a function.',
+            getComponentNameFromFiber(current),
+          );
+        }
+      }
     } else {
       ref.current = null;
     }
@@ -1077,19 +1091,32 @@ function commitAttachRef(finishedWork: Fiber) {
       instanceToUse = instance;
     }
     if (typeof ref === 'function') {
+      let retVal;
       if (
         enableProfilerTimer &&
         enableProfilerCommitHooks &&
         finishedWork.mode & ProfileMode
       ) {
         try {
           startLayoutEffectTimer();
-          ref(instanceToUse);
+          retVal = ref(instanceToUse);
         } finally {
           recordLayoutEffectDuration(finishedWork);
         }
       } else {
-        ref(instanceToUse);
+        retVal = ref(instanceToUse);
+      }
+      if (__DEV__) {
+        if (
+          warnAboutCallbackRefReturningFunction &&
+          typeof retVal === 'function'
+        ) {
+          console.error(
+            'Unexpected return value from a callback ref in %s. ' +
+              'A callback ref should not return a function.',
+            getComponentNameFromFiber(finishedWork),
+          );
+        }
       }
     } else {
       if (__DEV__) {

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -37,6 +37,7 @@ import {
   deletedTreeCleanUpLevel,
   enableSuspenseLayoutEffectSemantics,
   enableUpdaterTracking,
+  warnAboutCallbackRefReturningFunction,
 } from 'shared/ReactFeatureFlags';
 import {
   FunctionComponent,
@@ -250,6 +251,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
   const ref = current.ref;
   if (ref !== null) {
     if (typeof ref === 'function') {
+      let retVal;
       try {
         if (
           enableProfilerTimer &&
@@ -258,17 +260,29 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
         ) {
           try {
             startLayoutEffectTimer();
-            ref(null);
+            retVal = ref(null);
           } finally {
             recordLayoutEffectDuration(current);
           }
         } else {
-          ref(null);
+          retVal = ref(null);
         }
       } catch (error) {
         reportUncaughtErrorInDEV(error);
         captureCommitPhaseError(current, nearestMountedAncestor, error);
       }
+      if (__DEV__) {
+        if (
+          warnAboutCallbackRefReturningFunction &&
+          typeof retVal === 'function'
+        ) {
+          console.error(
+            'Unexpected return value from a callback ref in %s. ' +
+              'A callback ref should not return a function.',
+            getComponentNameFromFiber(current),
+          );
+        }
+      }
     } else {
       ref.current = null;
     }
@@ -1077,19 +1091,32 @@ function commitAttachRef(finishedWork: Fiber) {
       instanceToUse = instance;
     }
     if (typeof ref === 'function') {
+      let retVal;
       if (
         enableProfilerTimer &&
         enableProfilerCommitHooks &&
         finishedWork.mode & ProfileMode
       ) {
         try {
           startLayoutEffectTimer();
-          ref(instanceToUse);
+          retVal = ref(instanceToUse);
         } finally {
           recordLayoutEffectDuration(finishedWork);
         }
       } else {
-        ref(instanceToUse);
+        retVal = ref(instanceToUse);
+      }
+      if (__DEV__) {
+        if (
+          warnAboutCallbackRefReturningFunction &&
+          typeof retVal === 'function'
+        ) {
+          console.error(
+            'Unexpected return value from a callback ref in %s. ' +
+              'A callback ref should not return a function.',
+            getComponentNameFromFiber(finishedWork),
+          );
+        }
       }
     } else {
       if (__DEV__) {

diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -166,6 +166,8 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 
 export const enableUseRefAccessWarning = false;
 
+export const warnAboutCallbackRefReturningFunction = false;
+
 export const enableRecursiveCommitTraversal = false;
 
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -63,6 +63,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 export const enableStrictEffects = __DEV__;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 export const enableStrictEffects = false;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 export const enableStrictEffects = false;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js
@@ -53,6 +53,7 @@ export const warnOnSubscriptionInsideStartTransition = false;
 export const enableStrictEffects = false;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 export const enableStrictEffects = true;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 export const enableStrictEffects = false;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
 export const enableStrictEffects = false;
 export const createRootStrictEffectsByDefault = false;
 export const enableUseRefAccessWarning = false;
+export const warnAboutCallbackRefReturningFunction = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;

diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
@@ -19,6 +19,7 @@ export const enableFilterEmptyStringAttributesDOM = __VARIANT__;
 export const enableLegacyFBSupport = __VARIANT__;
 export const skipUnmountedBoundaries = __VARIANT__;
 export const enableUseRefAccessWarning = __VARIANT__;
+export const warnAboutCallbackRefReturningFunction = __VARIANT__;
 export const deletedTreeCleanUpLevel = __VARIANT__ ? 3 : 1;
 export const enableProfilerNestedUpdateScheduledHook = __VARIANT__;
 export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;

diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -64,6 +64,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const enableGetInspectorDataForInstanceInProduction = false;
 export const enableSuspenseServerRenderer = true;
 export const enableSelectiveHydration = true;
+export const warnAboutCallbackRefReturningFunction = true;
 
 export const enableLazyElements = true;
 export const enableCache = true;