Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ref cleanup function #25686

Merged
merged 7 commits into from
Nov 29, 2022
+195 −99
Merged

Add ref cleanup function #25686

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3716c39
Add ref cleanup function
sammy-SC Nov 16, 2022
be50757
Delete commitDetachRef
sammy-SC Nov 16, 2022
073cec2
yarn prettier
sammy-SC Nov 16, 2022
b9a8ed7
Move cleanup to finally branch
sammy-SC Nov 16, 2022
9d315ab
Remove typeof check
sammy-SC Nov 17, 2022
d68d6e9
Add unit test to cover conditionally returned clean up function
sammy-SC Nov 17, 2022
ebb8afc
Remove null check
sammy-SC Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 95 additions & 29 deletions packages/react-dom/src/__tests__/refs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,35 +351,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(<div ref={() => () => {}} />, container);
}).toErrorDev('Unexpected return value from a callback ref in div');

// Cleanup should warn, too.
expect(() => {
ReactDOM.render(<span />, container);
}).toErrorDev('Unexpected return value from a callback ref in div', {
withoutStack: true,
});

// No warning when returning non-functions.
ReactDOM.render(<p ref={() => ({})} />, container);
ReactDOM.render(<p ref={() => null} />, container);
ReactDOM.render(<p ref={() => undefined} />, container);

// Still warns on functions (not deduped).
expect(() => {
ReactDOM.render(<div ref={() => () => {}} />, 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', () => {
Expand Down Expand Up @@ -534,3 +505,98 @@ 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(
<div
ref={_ref => {
setup(_ref);
return cleanUp;
}}
/>,
container,
);

ReactDOM.render(
<div
ref={_ref => {
setup(_ref);
}}
/>,
container,
);

expect(setup).toHaveBeenCalledTimes(2);
expect(cleanUp).toHaveBeenCalledTimes(1);
expect(cleanUp.mock.calls[0][0]).toBe(undefined);

ReactDOM.render(<div ref={_ref => {}} />, 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(
<div
ref={_ref => {
setup(_ref);
return cleanUp;
}}
/>,
container,
);

expect(setup).toHaveBeenCalledTimes(1);
expect(cleanUp).toHaveBeenCalledTimes(0);

ReactDOM.render(
<div
ref={_ref => {
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(<div ref={_onRefChange} />, container);

expect(setup).toHaveBeenCalledTimes(1);
expect(cleanUp).toHaveBeenCalledTimes(0);

ReactDOM.render(
<div className="niceClassName" ref={_onRefChange} />,
container,
);

expect(setup).toHaveBeenCalledTimes(1);
expect(cleanUp).toHaveBeenCalledTimes(0);

ReactDOM.render(<div />, container);

expect(setup).toHaveBeenCalledTimes(1);
expect(cleanUp).toHaveBeenCalledTimes(1);
});
});
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiber.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ function FiberNode(
this.index = 0;

this.ref = null;
this.refCleanup = null;

this.pendingProps = pendingProps;
this.memoizedProps = null;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions packages/react-reconciler/src/ReactFiber.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ function FiberNode(
this.index = 0;

this.ref = null;
this.refCleanup = null;

this.pendingProps = pendingProps;
this.memoizedProps = null;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
61 changes: 30 additions & 31 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,33 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {

function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
const ref = current.ref;
if (ref !== null) {
const refCleanup = current.refCleanup;

if (refCleanup !== null) {
if (typeof ref === '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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we set refCleanup to null? What is finishedWork? Is it possible that finishedWork contains a ref that we have not yet detached?

}
}
}
} else if (ref !== null) {
if (typeof ref === 'function') {
let retVal;
try {
Expand Down Expand Up @@ -1583,14 +1609,9 @@ function commitAttachRef(finishedWork: Fiber) {
} 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),
);
}

if (typeof retVal === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally it's good to check the type as close to where you get it from the user as possible but it'd probably be ok to do this type check during the cleanup instead.

That way there's no extra work on initial mount.

The VM has to check the type anyway before calling it upon cleanup.

finishedWork.refCleanup = retVal;
}
} else {
if (__DEV__) {
Expand All @@ -1609,27 +1630,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.
Expand Down Expand Up @@ -4450,7 +4450,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
export {
commitPlacement,
commitAttachRef,
commitDetachRef,
invokeLayoutEffectMountInDEV,
invokeLayoutEffectUnmountInDEV,
invokePassiveEffectMountInDEV,
Expand Down
61 changes: 30 additions & 31 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,33 @@ function safelyAttachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {

function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) {
const ref = current.ref;
if (ref !== null) {
const refCleanup = current.refCleanup;

if (refCleanup !== null) {
if (typeof ref === '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 (ref !== null) {
if (typeof ref === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can place the if (refCleanup !== null) { inside here since you'll always do that check anyway so there's no extra checks. That way when it's not a function but a ref.current you don't do extra checks.

let retVal;
try {
Expand Down Expand Up @@ -1583,14 +1609,9 @@ function commitAttachRef(finishedWork: Fiber) {
} 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),
);
}

if (typeof retVal === 'function') {
finishedWork.refCleanup = retVal;
}
} else {
if (__DEV__) {
Expand All @@ -1609,27 +1630,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.
Expand Down Expand Up @@ -4450,7 +4450,6 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
export {
commitPlacement,
commitAttachRef,
commitDetachRef,
invokeLayoutEffectMountInDEV,
invokeLayoutEffectUnmountInDEV,
invokePassiveEffectMountInDEV,
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactInternalTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down