Skip to content

Commit e982254

Browse files
sammy-SCsammy-SC
authored
Add ref cleanup function (#25686)
Add option for ref function to return a clean up function. ```jsx <div ref={(_ref) => { // Use `_ref` return () => { // Clean up _ref }; }} /> ``` If clean up function is not provided. Ref function is called with null like it has been before. ```jsx <div ref={(_ref) => { if (_ref) { // Use _ref } else { // Clean up _ref } }} /> ```
1 parent edbfc63 commit e982254

File tree

6 files changed

+195
-99
lines changed

6 files changed

+195
-99
lines changed

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

Lines changed: 133 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -401,35 +401,6 @@ describe('ref swapping', () => {
401401
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
402402
);
403403
});
404-
405-
it('should warn about callback refs returning a function', () => {
406-
const container = document.createElement('div');
407-
expect(() => {
408-
ReactDOM.render(<div ref={() => () => {}} />, container);
409-
}).toErrorDev('Unexpected return value from a callback ref in div');
410-
411-
// Cleanup should warn, too.
412-
expect(() => {
413-
ReactDOM.render(<span />, container);
414-
}).toErrorDev('Unexpected return value from a callback ref in div', {
415-
withoutStack: true,
416-
});
417-
418-
// No warning when returning non-functions.
419-
ReactDOM.render(<p ref={() => ({})} />, container);
420-
ReactDOM.render(<p ref={() => null} />, container);
421-
ReactDOM.render(<p ref={() => undefined} />, container);
422-
423-
// Still warns on functions (not deduped).
424-
expect(() => {
425-
ReactDOM.render(<div ref={() => () => {}} />, container);
426-
}).toErrorDev('Unexpected return value from a callback ref in div');
427-
expect(() => {
428-
ReactDOM.unmountComponentAtNode(container);
429-
}).toErrorDev('Unexpected return value from a callback ref in div', {
430-
withoutStack: true,
431-
});
432-
});
433404
});
434405

435406
describe('root level refs', () => {
@@ -612,3 +583,136 @@ describe('strings refs across renderers', () => {
612583
expect(inst.refs.child2).toBe(div2.firstChild);
613584
});
614585
});
586+
587+
describe('refs return clean up function', () => {
588+
it('calls clean up function if it exists', () => {
589+
const container = document.createElement('div');
590+
let cleanUp = jest.fn();
591+
let setup = jest.fn();
592+
593+
ReactDOM.render(
594+
<div
595+
ref={_ref => {
596+
setup(_ref);
597+
return cleanUp;
598+
}}
599+
/>,
600+
container,
601+
);
602+
603+
ReactDOM.render(
604+
<div
605+
ref={_ref => {
606+
setup(_ref);
607+
}}
608+
/>,
609+
container,
610+
);
611+
612+
expect(setup).toHaveBeenCalledTimes(2);
613+
expect(cleanUp).toHaveBeenCalledTimes(1);
614+
expect(cleanUp.mock.calls[0][0]).toBe(undefined);
615+
616+
ReactDOM.render(<div ref={_ref => {}} />, container);
617+
618+
expect(cleanUp).toHaveBeenCalledTimes(1);
619+
expect(setup).toHaveBeenCalledTimes(3);
620+
expect(setup.mock.calls[2][0]).toBe(null);
621+
622+
cleanUp = jest.fn();
623+
setup = jest.fn();
624+
625+
ReactDOM.render(
626+
<div
627+
ref={_ref => {
628+
setup(_ref);
629+
return cleanUp;
630+
}}
631+
/>,
632+
container,
633+
);
634+
635+
expect(setup).toHaveBeenCalledTimes(1);
636+
expect(cleanUp).toHaveBeenCalledTimes(0);
637+
638+
ReactDOM.render(
639+
<div
640+
ref={_ref => {
641+
setup(_ref);
642+
return cleanUp;
643+
}}
644+
/>,
645+
container,
646+
);
647+
648+
expect(setup).toHaveBeenCalledTimes(2);
649+
expect(cleanUp).toHaveBeenCalledTimes(1);
650+
});
651+
652+
it('handles ref functions with stable identity', () => {
653+
const container = document.createElement('div');
654+
const cleanUp = jest.fn();
655+
const setup = jest.fn();
656+
657+
function _onRefChange(_ref) {
658+
setup(_ref);
659+
return cleanUp;
660+
}
661+
662+
ReactDOM.render(<div ref={_onRefChange} />, container);
663+
664+
expect(setup).toHaveBeenCalledTimes(1);
665+
expect(cleanUp).toHaveBeenCalledTimes(0);
666+
667+
ReactDOM.render(
668+
<div className="niceClassName" ref={_onRefChange} />,
669+
container,
670+
);
671+
672+
expect(setup).toHaveBeenCalledTimes(1);
673+
expect(cleanUp).toHaveBeenCalledTimes(0);
674+
675+
ReactDOM.render(<div />, container);
676+
677+
expect(setup).toHaveBeenCalledTimes(1);
678+
expect(cleanUp).toHaveBeenCalledTimes(1);
679+
});
680+
681+
it('warns if clean up function is returned when called with null', () => {
682+
const container = document.createElement('div');
683+
const cleanUp = jest.fn();
684+
const setup = jest.fn();
685+
let returnCleanUp = false;
686+
687+
ReactDOM.render(
688+
<div
689+
ref={_ref => {
690+
setup(_ref);
691+
if (returnCleanUp) {
692+
return cleanUp;
693+
}
694+
}}
695+
/>,
696+
container,
697+
);
698+
699+
expect(setup).toHaveBeenCalledTimes(1);
700+
expect(cleanUp).toHaveBeenCalledTimes(0);
701+
702+
returnCleanUp = true;
703+
704+
expect(() => {
705+
ReactDOM.render(
706+
<div
707+
ref={_ref => {
708+
setup(_ref);
709+
if (returnCleanUp) {
710+
return cleanUp;
711+
}
712+
}}
713+
/>,
714+
container,
715+
);
716+
}).toErrorDev('Unexpected return value from a callback ref in div');
717+
});
718+
});

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

Lines changed: 3 additions & 0 deletions
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 = current.refCleanup;
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‎

Lines changed: 3 additions & 0 deletions
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 = current.refCleanup;
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‎

Lines changed: 27 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -286,8 +286,32 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
286286

287287
function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
288288
const ref = current.ref;
289+
const refCleanup = current.refCleanup;
290+
289291
if (ref !== null) {
290-
if (typeof ref === 'function') {
292+
if (typeof refCleanup === '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+
} finally {
307+
// `refCleanup` has been called. Nullify all references to it to prevent double invocation.
308+
current.refCleanup = null;
309+
const finishedWork = current.alternate;
310+
if (finishedWork != null) {
311+
finishedWork.refCleanup = null;
312+
}
313+
}
314+
} else if (typeof ref === 'function') {
291315
let retVal;
292316
try {
293317
if (shouldProfile(current)) {
@@ -1572,25 +1596,15 @@ function commitAttachRef(finishedWork: Fiber) {
15721596
instanceToUse = instance;
15731597
}
15741598
if (typeof ref === 'function') {
1575-
let retVal;
15761599
if (shouldProfile(finishedWork)) {
15771600
try {
15781601
startLayoutEffectTimer();
1579-
retVal = ref(instanceToUse);
1602+
finishedWork.refCleanup = ref(instanceToUse);
15801603
} finally {
15811604
recordLayoutEffectDuration(finishedWork);
15821605
}
15831606
} else {
1584-
retVal = ref(instanceToUse);
1585-
}
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-
}
1607+
finishedWork.refCleanup = ref(instanceToUse);
15941608
}
15951609
} else {
15961610
if (__DEV__) {
@@ -1609,27 +1623,6 @@ function commitAttachRef(finishedWork: Fiber) {
16091623
}
16101624
}
16111625

1612-
function commitDetachRef(current: Fiber) {
1613-
const currentRef = current.ref;
1614-
if (currentRef !== null) {
1615-
if (typeof currentRef === 'function') {
1616-
if (shouldProfile(current)) {
1617-
try {
1618-
startLayoutEffectTimer();
1619-
currentRef(null);
1620-
} finally {
1621-
recordLayoutEffectDuration(current);
1622-
}
1623-
} else {
1624-
currentRef(null);
1625-
}
1626-
} else {
1627-
// $FlowFixMe unable to narrow type to the non-function case
1628-
currentRef.current = null;
1629-
}
1630-
}
1631-
}
1632-
16331626
function detachFiberMutation(fiber: Fiber) {
16341627
// Cut off the return pointer to disconnect it from the tree.
16351628
// This enables us to detect and warn against state updates on an unmounted component.
@@ -4450,7 +4443,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
44504443
export {
44514444
commitPlacement,
44524445
commitAttachRef,
4453-
commitDetachRef,
44544446
invokeLayoutEffectMountInDEV,
44554447
invokeLayoutEffectUnmountInDEV,
44564448
invokePassiveEffectMountInDEV,

0 commit comments

Comments
 (0)