Skip to content

Commit

Permalink
Move ref type check to receiver (#28464)
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 authored Mar 1, 2024
1 parent bb4b147 commit 2f8f776
Show file tree
Hide file tree
Showing 6 changed files with 44 additions and 43 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
12 changes: 4 additions & 8 deletions packages/react-dom/src/__tests__/refs-test.js
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.',
'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
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
25 changes: 16 additions & 9 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
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
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
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.",
"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

0 comments on commit 2f8f776

Please sign in to comment.