From e4af34bd73dfd6b2bf619185378b41aa15396ea2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 29 Feb 2024 21:26:36 -0500 Subject: [PATCH] Move ref type check to receiver (#28464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/__tests__/ReactComponent-test.js | 7 +++-- packages/react-dom/src/__tests__/refs-test.js | 12 +++----- .../react-reconciler/src/ReactChildFiber.js | 29 +++++++------------ .../src/ReactFiberBeginWork.js | 25 ++++++++++------ .../src/__tests__/ReactFiberRefs-test.js | 12 +++++--- scripts/error-codes/codes.json | 2 +- 6 files changed, 44 insertions(+), 43 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index ffc36ebe862f5..b8b825690529b 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -38,7 +38,7 @@ 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); @@ -46,7 +46,10 @@ describe('ReactComponent', () => { act(() => { root.render(
); }), - ).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 () => { diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index a6b7d7fea76fc..a749e63b55bea 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -401,22 +401,20 @@ describe('ref swapping', () => { root.render(
); }); }).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(
); }); }).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(
); }); - }).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 @@ -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'); }); }); diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index ac3c175f2acb1..8b0c422e7a3de 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -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" + @@ -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" @@ -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, diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index dd839073403ab..0ed97e9973b14 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -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; + } } } diff --git a/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js b/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js index b753f9dd0731e..e46387d8cc7dd 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js @@ -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
; } @@ -129,9 +133,9 @@ describe('ReactFiberRefs', () => { } const root = ReactNoop.createRoot(); - await expect(async () => { - await expect(act(() => root.render())).rejects.toThrow(); - }).toErrorDev('String refs are no longer supported'); + await expect(act(() => root.render())).rejects.toThrow( + 'Expected ref to be a function', + ); expect(refProp).toBe('child'); }); }); diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 1471b340098a0..2a14f02dd03be 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -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",