From fb87afe5b6f00168b359c05c4fbd1b067267591c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 27 Feb 2024 17:14:46 -0500 Subject: [PATCH] Move ref type check to receiver 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 | 4 +-- .../react-reconciler/src/ReactChildFiber.js | 29 +++++++------------ .../src/ReactFiberBeginWork.js | 26 +++++++++++------ .../src/__tests__/ReactFiberRefs-test.js | 12 +++++--- 5 files changed, 42 insertions(+), 36 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..f12dc7e52b8aa 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -401,14 +401,14 @@ 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(() => { 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 3e7d3693ded9b..e87df807a486f 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -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; + } } } 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'); }); });