Skip to content

Commit

Permalink
Move ref type check to receiver
Browse files Browse the repository at this point in the history
The runtime contains a type check to determine if a user-provided ref
is a valid type — a function or object (or a string, when
`disableStringRefs` is off). This currently happens during child
reconciliation. This changes it to happen only when the ref is passed
to the component that the ref is being attached to.

This is a continuation of the "ref as prop" change — until you actually
pass a ref to a HostComponent, class, etc, ref is a normal prop that has
no special behavior.
  • Loading branch information
acdlite committed Feb 29, 2024
1 parent fb10a2c commit fb87afe
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 36 deletions.
7 changes: 5 additions & 2 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
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
4 changes: 2 additions & 2 deletions packages/react-dom/src/__tests__/refs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -401,14 +401,14 @@ 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.',
'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(() => {
Expand Down
29 changes: 10 additions & 19 deletions packages/react-reconciler/src/ReactChildFiber.js
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')
) {
// 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
26 changes: 17 additions & 9 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1026,16 +1026,24 @@ 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') {
// TODO: Remove "string" from error message.
throw new Error(
'Expected ref to be a function, a string, an object returned by React.createRef(), or 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
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');
});
});

0 comments on commit fb87afe

Please sign in to comment.