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

Move ref type check to receiver #28464

Merged
merged 2 commits into from
Mar 1, 2024
+44 −43
Merged

Move ref type check to receiver #28464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb87afe
Move ref type check to receiver
acdlite Feb 27, 2024
ea0180b
Remove "string" from invalid ref error message
acdlite Feb 27, 2024
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
7 changes: 5 additions & 2 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,18 @@ describe('ReactComponent', () => {
}).toThrowError(/Target container is not a DOM element./);
});

// @gate !disableStringRefs || !__DEV__
// @gate !disableStringRefs
it('should throw when supplying a string ref outside of render method', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await expect(
act(() => {
root.render(<div ref="badDiv" />);
}),
).rejects.toThrow();
).rejects.toThrow(
'Element ref was specified as a string (badDiv) but no owner ' +
'was set',
);
});

it('should throw (in dev) when children are mutated during render', async () => {
Expand Down
12 changes: 4 additions & 8 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 @@ -401,22 +401,20 @@ describe('ref swapping', () => {
root.render(<div ref={10} />);
});
}).rejects.toThrow(
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
Copy link

Choose a reason for hiding this comment

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

Hhg.

'Element ref was specified as a string (10) but no owner was set.',
);
await expect(async () => {
await act(() => {
root.render(<div ref={true} />);
});
}).rejects.toThrow(
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
'Element ref was specified as a string (true) but no owner was set.',
);
await expect(async () => {
await act(() => {
root.render(<div ref={Symbol('foo')} />);
});
}).rejects.toThrow(
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
);
}).rejects.toThrow('Expected ref to be a function');
});

// @gate !enableRefAsProp
Expand All @@ -434,9 +432,7 @@ describe('ref swapping', () => {
key: null,
});
});
}).rejects.toThrow(
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
);
}).rejects.toThrow('Expected ref to be a function');
});
});

Expand Down
29 changes: 10 additions & 19 deletions packages/react-reconciler/src/ReactChildFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,17 +157,17 @@ function convertStringRefToCallbackRef(
returnFiber: Fiber,
current: Fiber | null,
element: ReactElement,
mixedRef: any,
mixedRef: string | number | boolean,
): CoercedStringRef {
if (__DEV__) {
checkPropStringCoercion(mixedRef, 'ref');
}
const stringRef = '' + (mixedRef: any);

const owner: ?Fiber = (element._owner: any);
if (!owner) {
if (typeof mixedRef !== 'string') {
throw new Error(
'Expected ref to be a function, a string, an object returned by React.createRef(), or null.',
);
}
throw new Error(
`Element ref was specified as a string (${mixedRef}) but no owner was set. This could happen for one of` +
`Element ref was specified as a string (${stringRef}) but no owner was set. This could happen for one of` +
' the following reasons:\n' +
'1. You may be adding a ref to a function component\n' +
"2. You may be adding a ref to a component that was not created inside a component's render method\n" +
Expand All @@ -184,13 +184,6 @@ function convertStringRefToCallbackRef(
);
}

// At this point, we know the ref isn't an object or function but it could
// be a number. Coerce it to a string.
if (__DEV__) {
checkPropStringCoercion(mixedRef, 'ref');
}
const stringRef = '' + mixedRef;

if (__DEV__) {
if (
// Will already warn with "Function components cannot be given refs"
Expand Down Expand Up @@ -267,12 +260,10 @@ function coerceRef(
let coercedRef;
if (
!disableStringRefs &&
mixedRef !== null &&
typeof mixedRef !== 'function' &&
typeof mixedRef !== 'object'
(typeof mixedRef === 'string' ||
typeof mixedRef === 'number' ||
typeof mixedRef === 'boolean')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a subtle change because we used to coerce any non-function, non-object type to a string, but now we explicitly check for only these types. It's only when string refs are not disabled, though.

) {
// Assume this is a string ref. If it's not, then this will throw an error
// to the user.
coercedRef = convertStringRefToCallbackRef(
returnFiber,
current,
Expand Down
25 changes: 16 additions & 9 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1026,16 +1026,23 @@ function updateProfiler(
}

function markRef(current: Fiber | null, workInProgress: Fiber) {
// TODO: This is also where we should check the type of the ref and error if
// an invalid one is passed, instead of during child reconcilation.
// TODO: Check props.ref instead of fiber.ref when enableRefAsProp is on.
const ref = workInProgress.ref;
if (
(current === null && ref !== null) ||
(current !== null && current.ref !== ref)
) {
// Schedule a Ref effect
workInProgress.flags |= Ref;
workInProgress.flags |= RefStatic;
if (ref === null) {
if (current !== null && current.ref !== null) {
// Schedule a Ref effect
workInProgress.flags |= Ref | RefStatic;
}
} else {
if (typeof ref !== 'function' && typeof ref !== 'object') {
throw new Error(
'Expected ref to be a function, an object returned by React.createRef(), or undefined/null.',
);
}
if (current === null || current.ref !== ref) {
// Schedule a Ref effect
workInProgress.flags |= Ref | RefStatic;
}
}
}

Expand Down
12 changes: 8 additions & 4 deletions packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,13 @@ describe('ReactFiberRefs', () => {
});

// @gate disableStringRefs
test('log an error in dev if a string ref is passed to a ref-receiving component', async () => {
test('throw if a string ref is passed to a ref-receiving component', async () => {
let refProp;
function Child({ref}) {
// This component renders successfully because the ref type check does not
// occur until you pass it to a component that accepts refs.
//
// So the div will throw, but not Child.
refProp = ref;
return <div ref={ref} />;
}
Expand All @@ -129,9 +133,9 @@ describe('ReactFiberRefs', () => {
}

const root = ReactNoop.createRoot();
await expect(async () => {
await expect(act(() => root.render(<Owner />))).rejects.toThrow();
}).toErrorDev('String refs are no longer supported');
await expect(act(() => root.render(<Owner />))).rejects.toThrow(
'Expected ref to be a function',
);
expect(refProp).toBe('child');
});
});
2 changes: 1 addition & 1 deletion scripts/error-codes/codes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@
"281": "Finished root should have a work-in-progress. This error is likely caused by a bug in React. Please file an issue.",
"282": "If the root does not have an updateQueue, we should have already bailed out. This error is likely caused by a bug in React. Please file an issue.",
"283": "Element type is invalid. Received a promise that resolves to: %s. Promise elements must resolve to a class or function.",
"284": "Expected ref to be a function, a string, an object returned by React.createRef(), or null.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we give this a new error code somehow so that older versions of React decode an error message with their respective ref type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did it this way intentionally for log continuity. The new message isn't incorrect for older versions, per se. It doesn't list "string" as a valid type but that's fine because we'd rather then use one of the other ones anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That said I also think it's fine if we make a new error code. Generally I try to avoid creating new ones if the change isn't significant, though.

"284": "Expected ref to be a function, an object returned by React.createRef(), or undefined/null.",
"285": "The root failed to unmount after an error. This is likely a bug in React. Please file an issue.",
"286": "%s(...): the first argument must be a React class instance. Instead received: %s.",
"287": "It is not supported to run the profiling version of a renderer (for example, `react-dom/profiling`) without also replacing the `schedule/tracking` module with `schedule/tracking-profiling`. Your bundler might have a setting for aliasing both modules. Learn more at https://reactjs.org/link/profiling",
Expand Down