Skip to content

Commit 67222f0

Browse files
gaearongaearon
authored
[Experiment] Warn if callback ref returns a function (#22313)
1 parent 50263d3 commit 67222f0

13 files changed

+103
-8
lines changed

‎packages/react-dom/src/__tests__/refs-test.js‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,36 @@ describe('ref swapping', () => {
351351
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
352352
);
353353
});
354+
355+
// @gate !__DEV__ || warnAboutCallbackRefReturningFunction
356+
it('should warn about callback refs returning a function', () => {
357+
const container = document.createElement('div');
358+
expect(() => {
359+
ReactDOM.render(<div ref={() => () => {}} />, container);
360+
}).toErrorDev('Unexpected return value from a callback ref in div');
361+
362+
// Cleanup should warn, too.
363+
expect(() => {
364+
ReactDOM.render(<span />, container);
365+
}).toErrorDev('Unexpected return value from a callback ref in div', {
366+
withoutStack: true,
367+
});
368+
369+
// No warning when returning non-functions.
370+
ReactDOM.render(<p ref={() => ({})} />, container);
371+
ReactDOM.render(<p ref={() => null} />, container);
372+
ReactDOM.render(<p ref={() => undefined} />, container);
373+
374+
// Still warns on functions (not deduped).
375+
expect(() => {
376+
ReactDOM.render(<div ref={() => () => {}} />, container);
377+
}).toErrorDev('Unexpected return value from a callback ref in div');
378+
expect(() => {
379+
ReactDOM.unmountComponentAtNode(container);
380+
}).toErrorDev('Unexpected return value from a callback ref in div', {
381+
withoutStack: true,
382+
});
383+
});
354384
});
355385

356386
describe('root level refs', () => {

‎packages/react-reconciler/src/ReactFiberCommitWork.new.js‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
deletedTreeCleanUpLevel,
3838
enableSuspenseLayoutEffectSemantics,
3939
enableUpdaterTracking,
40+
warnAboutCallbackRefReturningFunction,
4041
} from 'shared/ReactFeatureFlags';
4142
import {
4243
FunctionComponent,
@@ -250,6 +251,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
250251
const ref = current.ref;
251252
if (ref !== null) {
252253
if (typeof ref === 'function') {
254+
let retVal;
253255
try {
254256
if (
255257
enableProfilerTimer &&
@@ -258,17 +260,29 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
258260
) {
259261
try {
260262
startLayoutEffectTimer();
261-
ref(null);
263+
retVal = ref(null);
262264
} finally {
263265
recordLayoutEffectDuration(current);
264266
}
265267
} else {
266-
ref(null);
268+
retVal = ref(null);
267269
}
268270
} catch (error) {
269271
reportUncaughtErrorInDEV(error);
270272
captureCommitPhaseError(current, nearestMountedAncestor, error);
271273
}
274+
if (__DEV__) {
275+
if (
276+
warnAboutCallbackRefReturningFunction &&
277+
typeof retVal === 'function'
278+
) {
279+
console.error(
280+
'Unexpected return value from a callback ref in %s. ' +
281+
'A callback ref should not return a function.',
282+
getComponentNameFromFiber(current),
283+
);
284+
}
285+
}
272286
} else {
273287
ref.current = null;
274288
}
@@ -1077,19 +1091,32 @@ function commitAttachRef(finishedWork: Fiber) {
10771091
instanceToUse = instance;
10781092
}
10791093
if (typeof ref === 'function') {
1094+
let retVal;
10801095
if (
10811096
enableProfilerTimer &&
10821097
enableProfilerCommitHooks &&
10831098
finishedWork.mode & ProfileMode
10841099
) {
10851100
try {
10861101
startLayoutEffectTimer();
1087-
ref(instanceToUse);
1102+
retVal = ref(instanceToUse);
10881103
} finally {
10891104
recordLayoutEffectDuration(finishedWork);
10901105
}
10911106
} else {
1092-
ref(instanceToUse);
1107+
retVal = ref(instanceToUse);
1108+
}
1109+
if (__DEV__) {
1110+
if (
1111+
warnAboutCallbackRefReturningFunction &&
1112+
typeof retVal === 'function'
1113+
) {
1114+
console.error(
1115+
'Unexpected return value from a callback ref in %s. ' +
1116+
'A callback ref should not return a function.',
1117+
getComponentNameFromFiber(finishedWork),
1118+
);
1119+
}
10931120
}
10941121
} else {
10951122
if (__DEV__) {

‎packages/react-reconciler/src/ReactFiberCommitWork.old.js‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import {
3737
deletedTreeCleanUpLevel,
3838
enableSuspenseLayoutEffectSemantics,
3939
enableUpdaterTracking,
40+
warnAboutCallbackRefReturningFunction,
4041
} from 'shared/ReactFeatureFlags';
4142
import {
4243
FunctionComponent,
@@ -250,6 +251,7 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
250251
const ref = current.ref;
251252
if (ref !== null) {
252253
if (typeof ref === 'function') {
254+
let retVal;
253255
try {
254256
if (
255257
enableProfilerTimer &&
@@ -258,17 +260,29 @@ function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
258260
) {
259261
try {
260262
startLayoutEffectTimer();
261-
ref(null);
263+
retVal = ref(null);
262264
} finally {
263265
recordLayoutEffectDuration(current);
264266
}
265267
} else {
266-
ref(null);
268+
retVal = ref(null);
267269
}
268270
} catch (error) {
269271
reportUncaughtErrorInDEV(error);
270272
captureCommitPhaseError(current, nearestMountedAncestor, error);
271273
}
274+
if (__DEV__) {
275+
if (
276+
warnAboutCallbackRefReturningFunction &&
277+
typeof retVal === 'function'
278+
) {
279+
console.error(
280+
'Unexpected return value from a callback ref in %s. ' +
281+
'A callback ref should not return a function.',
282+
getComponentNameFromFiber(current),
283+
);
284+
}
285+
}
272286
} else {
273287
ref.current = null;
274288
}
@@ -1077,19 +1091,32 @@ function commitAttachRef(finishedWork: Fiber) {
10771091
instanceToUse = instance;
10781092
}
10791093
if (typeof ref === 'function') {
1094+
let retVal;
10801095
if (
10811096
enableProfilerTimer &&
10821097
enableProfilerCommitHooks &&
10831098
finishedWork.mode & ProfileMode
10841099
) {
10851100
try {
10861101
startLayoutEffectTimer();
1087-
ref(instanceToUse);
1102+
retVal = ref(instanceToUse);
10881103
} finally {
10891104
recordLayoutEffectDuration(finishedWork);
10901105
}
10911106
} else {
1092-
ref(instanceToUse);
1107+
retVal = ref(instanceToUse);
1108+
}
1109+
if (__DEV__) {
1110+
if (
1111+
warnAboutCallbackRefReturningFunction &&
1112+
typeof retVal === 'function'
1113+
) {
1114+
console.error(
1115+
'Unexpected return value from a callback ref in %s. ' +
1116+
'A callback ref should not return a function.',
1117+
getComponentNameFromFiber(finishedWork),
1118+
);
1119+
}
10931120
}
10941121
} else {
10951122
if (__DEV__) {

‎packages/shared/ReactFeatureFlags.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,8 @@ export const deferRenderPhaseUpdateToNextBatch = false;
166166

167167
export const enableUseRefAccessWarning = false;
168168

169+
export const warnAboutCallbackRefReturningFunction = false;
170+
169171
export const enableRecursiveCommitTraversal = false;
170172

171173
export const disableSchedulerTimeoutInWorkLoop = false;

‎packages/shared/forks/ReactFeatureFlags.native-fb.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
6363
export const enableStrictEffects = __DEV__;
6464
export const createRootStrictEffectsByDefault = false;
6565
export const enableUseRefAccessWarning = false;
66+
export const warnAboutCallbackRefReturningFunction = false;
6667

6768
export const enableRecursiveCommitTraversal = false;
6869
export const disableSchedulerTimeoutInWorkLoop = false;

‎packages/shared/forks/ReactFeatureFlags.native-oss.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
5454
export const enableStrictEffects = false;
5555
export const createRootStrictEffectsByDefault = false;
5656
export const enableUseRefAccessWarning = false;
57+
export const warnAboutCallbackRefReturningFunction = false;
5758

5859
export const enableRecursiveCommitTraversal = false;
5960
export const disableSchedulerTimeoutInWorkLoop = false;

‎packages/shared/forks/ReactFeatureFlags.test-renderer.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
5454
export const enableStrictEffects = false;
5555
export const createRootStrictEffectsByDefault = false;
5656
export const enableUseRefAccessWarning = false;
57+
export const warnAboutCallbackRefReturningFunction = false;
5758

5859
export const enableRecursiveCommitTraversal = false;
5960
export const disableSchedulerTimeoutInWorkLoop = false;

‎packages/shared/forks/ReactFeatureFlags.test-renderer.native.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ export const warnOnSubscriptionInsideStartTransition = false;
5353
export const enableStrictEffects = false;
5454
export const createRootStrictEffectsByDefault = false;
5555
export const enableUseRefAccessWarning = false;
56+
export const warnAboutCallbackRefReturningFunction = false;
5657

5758
export const enableRecursiveCommitTraversal = false;
5859
export const disableSchedulerTimeoutInWorkLoop = false;

‎packages/shared/forks/ReactFeatureFlags.test-renderer.www.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
5454
export const enableStrictEffects = true;
5555
export const createRootStrictEffectsByDefault = false;
5656
export const enableUseRefAccessWarning = false;
57+
export const warnAboutCallbackRefReturningFunction = false;
5758

5859
export const enableRecursiveCommitTraversal = false;
5960
export const disableSchedulerTimeoutInWorkLoop = false;

‎packages/shared/forks/ReactFeatureFlags.testing.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ export const deferRenderPhaseUpdateToNextBatch = false;
5454
export const enableStrictEffects = false;
5555
export const createRootStrictEffectsByDefault = false;
5656
export const enableUseRefAccessWarning = false;
57+
export const warnAboutCallbackRefReturningFunction = false;
5758

5859
export const enableRecursiveCommitTraversal = false;
5960
export const disableSchedulerTimeoutInWorkLoop = false;

0 commit comments

Comments
 (0)