From 6fda8e156faec5ea31002e11a95eab1db8512be0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 14 Sep 2021 17:03:34 +0100 Subject: [PATCH] [Experiment] Warn if callback ref returns a function --- packages/react-dom/src/__tests__/refs-test.js | 30 ++++++++++++++++ .../src/ReactFiberCommitWork.new.js | 35 ++++++++++++++++--- .../src/ReactFiberCommitWork.old.js | 35 ++++++++++++++++--- packages/shared/ReactFeatureFlags.js | 2 ++ .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 13 files changed, 103 insertions(+), 8 deletions(-) diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 4b616c1ae4cfa..872ff6e7dc2b6 100644 --- 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 !__DEV__ || warnAboutCallbackRefReturningFunction + it('should warn about callback refs returning a function', () => { + const container = document.createElement('div'); + expect(() => { + ReactDOM.render(
() => {}} />, container); + }).toErrorDev('Unexpected return value from a callback ref in div'); + + // Cleanup should warn, too. + expect(() => { + ReactDOM.render(, container); + }).toErrorDev('Unexpected return value from a callback ref in div', { + withoutStack: true, + }); + + // No warning when returning non-functions. + ReactDOM.render(

({})} />, container); + ReactDOM.render(

null} />, container); + ReactDOM.render(

undefined} />, container); + + // Still warns on functions (not deduped). + expect(() => { + ReactDOM.render(

() => {}} />, 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 index 85b5b778ba9f0..db46978eb2c69 100644 --- 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,6 +1091,7 @@ function commitAttachRef(finishedWork: Fiber) { instanceToUse = instance; } if (typeof ref === 'function') { + let retVal; if ( enableProfilerTimer && enableProfilerCommitHooks && @@ -1084,12 +1099,24 @@ function commitAttachRef(finishedWork: Fiber) { ) { 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 index 9613f0b168406..e0131080a32fa 100644 --- 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,6 +1091,7 @@ function commitAttachRef(finishedWork: Fiber) { instanceToUse = instance; } if (typeof ref === 'function') { + let retVal; if ( enableProfilerTimer && enableProfilerCommitHooks && @@ -1084,12 +1099,24 @@ function commitAttachRef(finishedWork: Fiber) { ) { 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 index f6eb72375f801..b9adc3b247617 100644 --- 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 index 41122a6c5ff30..a16e267d34f8f 100644 --- 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 index 0bfe6f3fecc76..5e6e925e8d25b 100644 --- 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 index c0b6eda1b0387..a74e056faf5bc 100644 --- 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 index 03d7ed0734d06..028755cbe0cd3 100644 --- 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 index 1618decf3843e..103d02e8a6316 100644 --- 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 index cca9a0d719325..0f9e710eda92e 100644 --- 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 index e1b633538cdee..740377f5aee86 100644 --- 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 index d6a5c77b837d1..6400ef0422503 100644 --- 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 index cd66a719143a3..3522244a3e13c 100644 --- 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;