From e98225485a124e35abc4cea82e6da944472ce7c7 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Tue, 29 Nov 2022 16:41:02 +0000 Subject: [PATCH] Add ref cleanup function (#25686) Add option for ref function to return a clean up function. ```jsx
{ // 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
{ if (_ref) { // Use _ref } else { // Clean up _ref } }} /> ``` --- packages/react-dom/src/__tests__/refs-test.js | 162 ++++++++++++++---- .../react-reconciler/src/ReactFiber.new.js | 3 + .../react-reconciler/src/ReactFiber.old.js | 3 + .../src/ReactFiberCommitWork.new.js | 62 +++---- .../src/ReactFiberCommitWork.old.js | 62 +++---- .../src/ReactInternalTypes.js | 2 + 6 files changed, 195 insertions(+), 99 deletions(-) diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 9d1965c1a465a..363005f06658c 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -401,35 +401,6 @@ describe('ref swapping', () => { 'Expected ref to be a function, a string, an object returned by React.createRef(), or null.', ); }); - - 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', () => { @@ -612,3 +583,136 @@ describe('strings refs across renderers', () => { expect(inst.refs.child2).toBe(div2.firstChild); }); }); + +describe('refs return clean up function', () => { + it('calls clean up function if it exists', () => { + const container = document.createElement('div'); + let cleanUp = jest.fn(); + let setup = jest.fn(); + + ReactDOM.render( +
{ + setup(_ref); + return cleanUp; + }} + />, + container, + ); + + ReactDOM.render( +
{ + setup(_ref); + }} + />, + container, + ); + + expect(setup).toHaveBeenCalledTimes(2); + expect(cleanUp).toHaveBeenCalledTimes(1); + expect(cleanUp.mock.calls[0][0]).toBe(undefined); + + ReactDOM.render(
{}} />, container); + + expect(cleanUp).toHaveBeenCalledTimes(1); + expect(setup).toHaveBeenCalledTimes(3); + expect(setup.mock.calls[2][0]).toBe(null); + + cleanUp = jest.fn(); + setup = jest.fn(); + + ReactDOM.render( +
{ + setup(_ref); + return cleanUp; + }} + />, + container, + ); + + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanUp).toHaveBeenCalledTimes(0); + + ReactDOM.render( +
{ + setup(_ref); + return cleanUp; + }} + />, + container, + ); + + expect(setup).toHaveBeenCalledTimes(2); + expect(cleanUp).toHaveBeenCalledTimes(1); + }); + + it('handles ref functions with stable identity', () => { + const container = document.createElement('div'); + const cleanUp = jest.fn(); + const setup = jest.fn(); + + function _onRefChange(_ref) { + setup(_ref); + return cleanUp; + } + + ReactDOM.render(
, container); + + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanUp).toHaveBeenCalledTimes(0); + + ReactDOM.render( +
, + container, + ); + + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanUp).toHaveBeenCalledTimes(0); + + ReactDOM.render(
, container); + + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanUp).toHaveBeenCalledTimes(1); + }); + + it('warns if clean up function is returned when called with null', () => { + const container = document.createElement('div'); + const cleanUp = jest.fn(); + const setup = jest.fn(); + let returnCleanUp = false; + + ReactDOM.render( +
{ + setup(_ref); + if (returnCleanUp) { + return cleanUp; + } + }} + />, + container, + ); + + expect(setup).toHaveBeenCalledTimes(1); + expect(cleanUp).toHaveBeenCalledTimes(0); + + returnCleanUp = true; + + expect(() => { + ReactDOM.render( +
{ + setup(_ref); + if (returnCleanUp) { + return cleanUp; + } + }} + />, + container, + ); + }).toErrorDev('Unexpected return value from a callback ref in div'); + }); +}); diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 7b1c29543eba3..9a60de9797392 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -148,6 +148,7 @@ function FiberNode( this.index = 0; this.ref = null; + this.refCleanup = null; this.pendingProps = pendingProps; this.memoizedProps = null; @@ -336,6 +337,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.sibling = current.sibling; workInProgress.index = current.index; workInProgress.ref = current.ref; + workInProgress.refCleanup = current.refCleanup; if (enableProfilerTimer) { workInProgress.selfBaseDuration = current.selfBaseDuration; @@ -880,6 +882,7 @@ export function assignFiberPropertiesInDEV( target.sibling = source.sibling; target.index = source.index; target.ref = source.ref; + target.refCleanup = source.refCleanup; target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index fd1468d4a5d7c..64297701cd472 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -148,6 +148,7 @@ function FiberNode( this.index = 0; this.ref = null; + this.refCleanup = null; this.pendingProps = pendingProps; this.memoizedProps = null; @@ -336,6 +337,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { workInProgress.sibling = current.sibling; workInProgress.index = current.index; workInProgress.ref = current.ref; + workInProgress.refCleanup = current.refCleanup; if (enableProfilerTimer) { workInProgress.selfBaseDuration = current.selfBaseDuration; @@ -880,6 +882,7 @@ export function assignFiberPropertiesInDEV( target.sibling = source.sibling; target.index = source.index; target.ref = source.ref; + target.refCleanup = source.refCleanup; target.pendingProps = source.pendingProps; target.memoizedProps = source.memoizedProps; target.updateQueue = source.updateQueue; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 1a9b72c8aeb0b..ae9337a01647f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -286,8 +286,32 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { const ref = current.ref; + const refCleanup = current.refCleanup; + if (ref !== null) { - if (typeof ref === 'function') { + if (typeof refCleanup === 'function') { + try { + if (shouldProfile(current)) { + try { + startLayoutEffectTimer(); + refCleanup(); + } finally { + recordLayoutEffectDuration(current); + } + } else { + refCleanup(); + } + } catch (error) { + captureCommitPhaseError(current, nearestMountedAncestor, error); + } finally { + // `refCleanup` has been called. Nullify all references to it to prevent double invocation. + current.refCleanup = null; + const finishedWork = current.alternate; + if (finishedWork != null) { + finishedWork.refCleanup = null; + } + } + } else if (typeof ref === 'function') { let retVal; try { if (shouldProfile(current)) { @@ -1572,25 +1596,15 @@ function commitAttachRef(finishedWork: Fiber) { instanceToUse = instance; } if (typeof ref === 'function') { - let retVal; if (shouldProfile(finishedWork)) { try { startLayoutEffectTimer(); - retVal = ref(instanceToUse); + finishedWork.refCleanup = ref(instanceToUse); } finally { recordLayoutEffectDuration(finishedWork); } } else { - retVal = ref(instanceToUse); - } - if (__DEV__) { - if (typeof retVal === 'function') { - console.error( - 'Unexpected return value from a callback ref in %s. ' + - 'A callback ref should not return a function.', - getComponentNameFromFiber(finishedWork), - ); - } + finishedWork.refCleanup = ref(instanceToUse); } } else { if (__DEV__) { @@ -1609,27 +1623,6 @@ function commitAttachRef(finishedWork: Fiber) { } } -function commitDetachRef(current: Fiber) { - const currentRef = current.ref; - if (currentRef !== null) { - if (typeof currentRef === 'function') { - if (shouldProfile(current)) { - try { - startLayoutEffectTimer(); - currentRef(null); - } finally { - recordLayoutEffectDuration(current); - } - } else { - currentRef(null); - } - } else { - // $FlowFixMe unable to narrow type to the non-function case - currentRef.current = null; - } - } -} - function detachFiberMutation(fiber: Fiber) { // Cut off the return pointer to disconnect it from the tree. // This enables us to detect and warn against state updates on an unmounted component. @@ -4450,7 +4443,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { export { commitPlacement, commitAttachRef, - commitDetachRef, invokeLayoutEffectMountInDEV, invokeLayoutEffectUnmountInDEV, invokePassiveEffectMountInDEV, diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index a873d23c3d729..be4d5cfb8d5d9 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -286,8 +286,32 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { const ref = current.ref; + const refCleanup = current.refCleanup; + if (ref !== null) { - if (typeof ref === 'function') { + if (typeof refCleanup === 'function') { + try { + if (shouldProfile(current)) { + try { + startLayoutEffectTimer(); + refCleanup(); + } finally { + recordLayoutEffectDuration(current); + } + } else { + refCleanup(); + } + } catch (error) { + captureCommitPhaseError(current, nearestMountedAncestor, error); + } finally { + // `refCleanup` has been called. Nullify all references to it to prevent double invocation. + current.refCleanup = null; + const finishedWork = current.alternate; + if (finishedWork != null) { + finishedWork.refCleanup = null; + } + } + } else if (typeof ref === 'function') { let retVal; try { if (shouldProfile(current)) { @@ -1572,25 +1596,15 @@ function commitAttachRef(finishedWork: Fiber) { instanceToUse = instance; } if (typeof ref === 'function') { - let retVal; if (shouldProfile(finishedWork)) { try { startLayoutEffectTimer(); - retVal = ref(instanceToUse); + finishedWork.refCleanup = ref(instanceToUse); } finally { recordLayoutEffectDuration(finishedWork); } } else { - retVal = ref(instanceToUse); - } - if (__DEV__) { - if (typeof retVal === 'function') { - console.error( - 'Unexpected return value from a callback ref in %s. ' + - 'A callback ref should not return a function.', - getComponentNameFromFiber(finishedWork), - ); - } + finishedWork.refCleanup = ref(instanceToUse); } } else { if (__DEV__) { @@ -1609,27 +1623,6 @@ function commitAttachRef(finishedWork: Fiber) { } } -function commitDetachRef(current: Fiber) { - const currentRef = current.ref; - if (currentRef !== null) { - if (typeof currentRef === 'function') { - if (shouldProfile(current)) { - try { - startLayoutEffectTimer(); - currentRef(null); - } finally { - recordLayoutEffectDuration(current); - } - } else { - currentRef(null); - } - } else { - // $FlowFixMe unable to narrow type to the non-function case - currentRef.current = null; - } - } -} - function detachFiberMutation(fiber: Fiber) { // Cut off the return pointer to disconnect it from the tree. // This enables us to detect and warn against state updates on an unmounted component. @@ -4450,7 +4443,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { export { commitPlacement, commitAttachRef, - commitDetachRef, invokeLayoutEffectMountInDEV, invokeLayoutEffectUnmountInDEV, invokePassiveEffectMountInDEV, diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index a5f28fd5d2450..0b7a0c4f9ef1f 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -131,6 +131,8 @@ export type Fiber = { | (((handle: mixed) => void) & {_stringRef: ?string, ...}) | RefObject, + refCleanup: null | (() => void), + // Input is the data coming into process this fiber. Arguments. Props. pendingProps: any, // This type will be more specific once we overload the tag. memoizedProps: any, // The props used to create the output.