Skip to content

Commit 608d9b6

Browse files
sammy-SCsammy-SC
committed
Add ref cleanup function
1 parent c54e354 commit 608d9b6

File tree

6 files changed

+126
-47
lines changed

6 files changed

+126
-47
lines changed

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

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

385356
describe('root level refs', () => {
@@ -534,3 +505,69 @@ describe('strings refs across renderers', () => {
534505
expect(inst.refs.child2).toBe(div2.firstChild);
535506
});
536507
});
508+
509+
describe('refs return clean up function', () => {
510+
it('does not break', () => {
511+
const container = document.createElement('div');
512+
let cleanUp = jest.fn();
513+
let setup = jest.fn();
514+
515+
ReactDOM.render(
516+
<div
517+
ref={_ref => {
518+
setup(_ref);
519+
return cleanUp;
520+
}}
521+
/>,
522+
container,
523+
);
524+
525+
ReactDOM.render(
526+
<div
527+
ref={_ref => {
528+
setup(_ref);
529+
}}
530+
/>,
531+
container,
532+
);
533+
534+
expect(setup).toHaveBeenCalledTimes(2);
535+
expect(cleanUp).toHaveBeenCalledTimes(1);
536+
expect(cleanUp.mock.calls[0][0]).toBe(undefined);
537+
538+
ReactDOM.render(<div ref={_ref => {}} />, container);
539+
540+
expect(cleanUp).toHaveBeenCalledTimes(1);
541+
expect(setup).toHaveBeenCalledTimes(3);
542+
expect(setup.mock.calls[2][0]).toBe(null);
543+
544+
cleanUp = jest.fn();
545+
setup = jest.fn();
546+
547+
ReactDOM.render(
548+
<div
549+
ref={_ref => {
550+
setup(_ref);
551+
return cleanUp;
552+
}}
553+
/>,
554+
container,
555+
);
556+
557+
expect(setup).toHaveBeenCalledTimes(1);
558+
expect(cleanUp).toHaveBeenCalledTimes(0);
559+
560+
ReactDOM.render(
561+
<div
562+
ref={_ref => {
563+
setup(_ref);
564+
return cleanUp;
565+
}}
566+
/>,
567+
container,
568+
);
569+
570+
expect(setup).toHaveBeenCalledTimes(2);
571+
expect(cleanUp).toHaveBeenCalledTimes(1);
572+
});
573+
});

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

+3
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ function FiberNode(
148148
this.index = 0;
149149

150150
this.ref = null;
151+
this.refCleanup = null;
151152

152153
this.pendingProps = pendingProps;
153154
this.memoizedProps = null;
@@ -336,6 +337,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
336337
workInProgress.sibling = current.sibling;
337338
workInProgress.index = current.index;
338339
workInProgress.ref = current.ref;
340+
workInProgress.refCleanup = null;
339341

340342
if (enableProfilerTimer) {
341343
workInProgress.selfBaseDuration = current.selfBaseDuration;
@@ -880,6 +882,7 @@ export function assignFiberPropertiesInDEV(
880882
target.sibling = source.sibling;
881883
target.index = source.index;
882884
target.ref = source.ref;
885+
target.refCleanup = source.refCleanup;
883886
target.pendingProps = source.pendingProps;
884887
target.memoizedProps = source.memoizedProps;
885888
target.updateQueue = source.updateQueue;

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

+3
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ function FiberNode(
148148
this.index = 0;
149149

150150
this.ref = null;
151+
this.refCleanup = null;
151152

152153
this.pendingProps = pendingProps;
153154
this.memoizedProps = null;
@@ -336,6 +337,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber {
336337
workInProgress.sibling = current.sibling;
337338
workInProgress.index = current.index;
338339
workInProgress.ref = current.ref;
340+
workInProgress.refCleanup = null;
339341

340342
if (enableProfilerTimer) {
341343
workInProgress.selfBaseDuration = current.selfBaseDuration;
@@ -880,6 +882,7 @@ export function assignFiberPropertiesInDEV(
880882
target.sibling = source.sibling;
881883
target.index = source.index;
882884
target.ref = source.ref;
885+
target.refCleanup = source.refCleanup;
883886
target.pendingProps = source.pendingProps;
884887
target.memoizedProps = source.memoizedProps;
885888
target.updateQueue = source.updateQueue;

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

+26-9
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,29 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
286286

287287
function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
288288
const ref = current.ref;
289-
if (ref !== null) {
289+
const refCleanup = current.refCleanup;
290+
291+
if (refCleanup !== null) {
292+
if (typeof ref === 'function') {
293+
try {
294+
if (shouldProfile(current)) {
295+
try {
296+
startLayoutEffectTimer();
297+
refCleanup();
298+
} finally {
299+
recordLayoutEffectDuration(current);
300+
}
301+
} else {
302+
refCleanup();
303+
}
304+
} catch (error) {
305+
captureCommitPhaseError(current, nearestMountedAncestor, error);
306+
}
307+
} else {
308+
// $FlowFixMe unable to narrow type to RefObject
309+
ref.refCleanup = null;
310+
}
311+
} else if (ref !== null) {
290312
if (typeof ref === 'function') {
291313
let retVal;
292314
try {
@@ -1583,14 +1605,9 @@ function commitAttachRef(finishedWork: Fiber) {
15831605
} else {
15841606
retVal = ref(instanceToUse);
15851607
}
1586-
if (__DEV__) {
1587-
if (typeof retVal === 'function') {
1588-
console.error(
1589-
'Unexpected return value from a callback ref in %s. ' +
1590-
'A callback ref should not return a function.',
1591-
getComponentNameFromFiber(finishedWork),
1592-
);
1593-
}
1608+
1609+
if (typeof retVal === 'function') {
1610+
finishedWork.refCleanup = retVal;
15941611
}
15951612
} else {
15961613
if (__DEV__) {

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

+26-9
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,29 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
286286

287287
function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
288288
const ref = current.ref;
289-
if (ref !== null) {
289+
const refCleanup = current.refCleanup;
290+
291+
if (refCleanup !== null) {
292+
if (typeof ref === 'function') {
293+
try {
294+
if (shouldProfile(current)) {
295+
try {
296+
startLayoutEffectTimer();
297+
refCleanup();
298+
} finally {
299+
recordLayoutEffectDuration(current);
300+
}
301+
} else {
302+
refCleanup();
303+
}
304+
} catch (error) {
305+
captureCommitPhaseError(current, nearestMountedAncestor, error);
306+
}
307+
} else {
308+
// $FlowFixMe unable to narrow type to RefObject
309+
ref.refCleanup = null;
310+
}
311+
} else if (ref !== null) {
290312
if (typeof ref === 'function') {
291313
let retVal;
292314
try {
@@ -1583,14 +1605,9 @@ function commitAttachRef(finishedWork: Fiber) {
15831605
} else {
15841606
retVal = ref(instanceToUse);
15851607
}
1586-
if (__DEV__) {
1587-
if (typeof retVal === 'function') {
1588-
console.error(
1589-
'Unexpected return value from a callback ref in %s. ' +
1590-
'A callback ref should not return a function.',
1591-
getComponentNameFromFiber(finishedWork),
1592-
);
1593-
}
1608+
1609+
if (typeof retVal === 'function') {
1610+
finishedWork.refCleanup = retVal;
15941611
}
15951612
} else {
15961613
if (__DEV__) {

‎packages/react-reconciler/src/ReactInternalTypes.js

+2
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ export type Fiber = {
131131
| (((handle: mixed) => void) & {_stringRef: ?string, ...})
132132
| RefObject,
133133

134+
refCleanup: null | (() => void),
135+
134136
// Input is the data coming into process this fiber. Arguments. Props.
135137
pendingProps: any, // This type will be more specific once we overload the tag.
136138
memoizedProps: any, // The props used to create the output.

0 commit comments

Comments
 (0)