From 9392a7e824b4a0784fcd92f9983d4139d1d896bd Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 29 Feb 2024 13:31:49 -0500 Subject: [PATCH] Move string ref coercion to JSX runtime This moves the entire string ref implementation out Fiber and into the JSX runtime. The string is converted to a callback ref during element creation. This is a subtle change in behavior, because it will have already been converted to a callback ref if you access element.prop.ref or element.ref. But this is only for Meta, because string refs are disabled entirely in open source. And if it leads to an issue in practice, the solution is to switch to a different ref type, which Meta is going to do regardless. --- .../src/__tests__/ReactComponent-test.js | 17 +- .../ReactDOMServerIntegrationRefs-test.js | 1 + .../ReactDeprecationWarnings-test.js | 9 +- .../__tests__/ReactFunctionComponent-test.js | 17 +- .../multiple-copies-of-react-test.js | 13 +- packages/react-dom/src/__tests__/refs-test.js | 73 ++++---- .../react-reconciler/src/ReactChildFiber.js | 156 +----------------- .../ReactIncrementalSideEffects-test.js | 1 + .../ReactCoffeeScriptClass-test.coffee | 2 +- .../react/src/__tests__/ReactES6Class-test.js | 2 +- .../src/__tests__/ReactElementClone-test.js | 10 +- .../src/__tests__/ReactStrictMode-test.js | 8 +- .../__tests__/ReactTypeScriptClass-test.ts | 2 +- packages/react/src/jsx/ReactJSXElement.js | 137 ++++++++++++++- 14 files changed, 199 insertions(+), 249 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index b8b825690529b..e76c581dd0516 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -38,7 +38,6 @@ describe('ReactComponent', () => { }).toThrowError(/Target container is not a DOM element./); }); - // @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,10 +45,10 @@ describe('ReactComponent', () => { act(() => { root.render(
); }), - ).rejects.toThrow( - 'Element ref was specified as a string (badDiv) but no owner ' + - 'was set', - ); + ) + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + .rejects.toThrow(); }); it('should throw (in dev) when children are mutated during render', async () => { @@ -168,18 +167,14 @@ describe('ReactComponent', () => { root.render(); }); }).toErrorDev([ - 'Warning: Component "div" contains the string ref "inner". ' + + 'Warning: Component "Component" contains the string ref "inner". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + + ' in Wrapper (at **)\n' + ' in div (at **)\n' + ' in Wrapper (at **)\n' + ' in Component (at **)', - 'Warning: Component "Component" contains the string ref "outer". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in Component (at **)', ]); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index 1bb505b589c23..25d5795f47d2d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -100,6 +100,7 @@ describe('ReactDOMServerIntegration', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + ' in RefsComponent (at **)', ]); expect(component.refs.myDiv).toBe(root.firstChild); diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js index 7064520dec5b3..5cdd986e57793 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js @@ -84,6 +84,7 @@ describe('ReactDeprecationWarnings', () => { 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: ' + 'https://reactjs.org/link/strict-mode-string-ref' + + '\n in RefComponent (at **)' + '\n in Component (at **)', ); }); @@ -135,10 +136,6 @@ describe('ReactDeprecationWarnings', () => { 'We ask you to manually fix this case by using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: ' + 'https://reactjs.org/link/strict-mode-string-ref', - 'Warning: Component "Component" contains the string ref "refComponent". ' + - 'Support for string refs will be removed in a future major release. We recommend ' + - 'using useRef() or createRef() instead. Learn more about using refs safely here: ' + - 'https://reactjs.org/link/strict-mode-string-ref', ]); }); @@ -171,10 +168,6 @@ describe('ReactDeprecationWarnings', () => { 'We ask you to manually fix this case by using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: ' + 'https://reactjs.org/link/strict-mode-string-ref', - 'Warning: Component "Component" contains the string ref "refComponent". ' + - 'Support for string refs will be removed in a future major release. We recommend ' + - 'using useRef() or createRef() instead. Learn more about using refs safely here: ' + - 'https://reactjs.org/link/strict-mode-string-ref', ]); }); }); diff --git a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js index e73765257dc88..9704f9fbdf32a 100644 --- a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js @@ -173,7 +173,6 @@ describe('ReactFunctionComponent', () => { ).resolves.not.toThrowError(); }); - // @gate !disableStringRefs it('should throw on string refs in pure functions', async () => { function Child() { return
; @@ -185,18 +184,10 @@ describe('ReactFunctionComponent', () => { act(() => { root.render(); }), - ).rejects.toThrowError( - __DEV__ - ? 'Function components cannot have string refs. We recommend using useRef() instead.' - : // It happens because we don't save _owner in production for - // function components. - 'Element ref was specified as a string (me) 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" + - '3. You have multiple copies of React loaded\n' + - 'See https://reactjs.org/link/refs-must-have-owner for more information.', - ); + ) + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + .rejects.toThrowError(); }); // @gate !enableRefAsProp || !__DEV__ diff --git a/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js b/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js index 1afb3d35e7cbf..296bcb82ba097 100644 --- a/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js +++ b/packages/react-dom/src/__tests__/multiple-copies-of-react-test.js @@ -22,7 +22,6 @@ class TextWithStringRef extends React.Component { } describe('when different React version is used with string ref', () => { - // @gate !disableStringRefs it('throws the "Refs must have owner" warning', async () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); @@ -30,13 +29,9 @@ describe('when different React version is used with string ref', () => { act(() => { root.render(); }), - ).rejects.toThrow( - 'Element ref was specified as a string (foo) 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" + - '3. You have multiple copies of React loaded\n' + - 'See https://reactjs.org/link/refs-must-have-owner for more information.', - ); + ) + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + .rejects.toThrow(); }); }); diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index a749e63b55bea..430685f91f6ed 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -131,22 +131,22 @@ describe('reactiverefs', () => { ); }); }).toErrorDev([ - 'Warning: Component "div" contains the string ref "resetDiv". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in div (at **)\n' + - ' in TestRefsComponent (at **)', - 'Warning: Component "span" contains the string ref "clickLog0". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in span (at **)\n' + - ' in ClickCounter (at **)\n' + + 'Warning: Component "TestRefsComponent" contains the string ' + + 'ref "resetDiv". Support for string refs will be removed in a ' + + 'future major release. We recommend using useRef() or createRef() ' + + 'instead. Learn more about using refs safely ' + + 'here: https://reactjs.org/link/strict-mode-string-ref\n' + ' in div (at **)\n' + - ' in GeneralContainerComponent (at **)\n' + ' in div (at **)\n' + ' in TestRefsComponent (at **)', + 'Warning: Component "ClickCounter" contains the string ' + + 'ref "clickLog0". Support for string refs will be removed in a ' + + 'future major release. We recommend using useRef() or createRef() ' + + 'instead. Learn more about using refs safely ' + + 'here: https://reactjs.org/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + + ' in span (at **)\n' + + ' in ClickCounter (at **)', ]); expect(testRefsComponent instanceof TestRefsComponent).toBe(true); @@ -387,12 +387,12 @@ describe('ref swapping', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + ' in A (at **)', ]); expect(a.refs[1].nodeName).toBe('DIV'); }); - // @gate !disableStringRefs it('provides an error for invalid refs', async () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); @@ -400,16 +400,16 @@ describe('ref swapping', () => { await act(() => { root.render(
); }); - }).rejects.toThrow( - 'Element ref was specified as a string (10) but no owner was set.', - ); + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + }).rejects.toThrow(); await expect(async () => { await act(() => { root.render(
); }); - }).rejects.toThrow( - 'Element ref was specified as a string (true) but no owner was set.', - ); + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + }).rejects.toThrow(); await expect(async () => { await act(() => { root.render(
); @@ -546,7 +546,6 @@ describe('creating element with string ref in constructor', () => { } } - // @gate !disableStringRefs it('throws an error', async () => { await expect(async function () { const container = document.createElement('div'); @@ -555,14 +554,9 @@ describe('creating element with string ref in constructor', () => { await act(() => { root.render(); }); - }).rejects.toThrowError( - 'Element ref was specified as a string (p) 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" + - '3. You have multiple copies of React loaded\n' + - 'See https://reactjs.org/link/refs-must-have-owner for more information.', - ); + // TODO: This throws an AggregateError. Need to update test infra to + // support matching against AggregateError. + }).rejects.toThrowError(); }); }); @@ -616,10 +610,11 @@ describe('strings refs across renderers', () => { ); }); }).toErrorDev([ - 'Warning: Component "Indirection" contains the string ref "child1". ' + + 'Warning: Component "Parent" contains the string ref "child1". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + + ' in div (at **)\n' + ' in Indirection (at **)\n' + ' in Parent (at **)', ]); @@ -628,20 +623,10 @@ describe('strings refs across renderers', () => { expect(inst.refs.child1.tagName).toBe('DIV'); expect(inst.refs.child1).toBe(div1.firstChild); - await expect(async () => { - // Now both refs should be rendered. - await act(() => { - root.render(); - }); - }).toErrorDev( - [ - 'Warning: Component "Root" contains the string ref "child2". ' + - 'Support for string refs will be removed in a future major release. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref', - ], - {withoutStack: true}, - ); + // Now both refs should be rendered. + await act(() => { + root.render(); + }); expect(inst.refs.child1.tagName).toBe('DIV'); expect(inst.refs.child1).toBe(div1.firstChild); expect(inst.refs.child2.tagName).toBe('DIV'); diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 8b0c422e7a3de..b327c559a089e 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -34,17 +34,9 @@ import { REACT_LAZY_TYPE, REACT_CONTEXT_TYPE, } from 'shared/ReactSymbols'; -import { - ClassComponent, - HostRoot, - HostText, - HostPortal, - Fragment, -} from './ReactWorkTags'; +import {HostRoot, HostText, HostPortal, Fragment} from './ReactWorkTags'; import isArray from 'shared/isArray'; -import assign from 'shared/assign'; -import {checkPropStringCoercion} from 'shared/CheckStringCoercion'; -import {enableRefAsProp, disableStringRefs} from 'shared/ReactFeatureFlags'; +import {enableRefAsProp} from 'shared/ReactFeatureFlags'; import { createWorkInProgress, @@ -85,7 +77,6 @@ function mergeDebugInfo( let didWarnAboutMaps; let didWarnAboutGenerators; -let didWarnAboutStringRefs; let ownerHasKeyUseWarning; let ownerHasFunctionTypeWarning; let ownerHasSymbolTypeWarning; @@ -94,7 +85,6 @@ let warnForMissingKey = (child: mixed, returnFiber: Fiber) => {}; if (__DEV__) { didWarnAboutMaps = false; didWarnAboutGenerators = false; - didWarnAboutStringRefs = ({}: {[string]: boolean}); /** * Warn if there's no key explicitly set on dynamic arrays of children or @@ -138,10 +128,6 @@ if (__DEV__) { }; } -function isReactClass(type: any) { - return type.prototype && type.prototype.isReactComponent; -} - function unwrapThenable(thenable: Thenable): T { const index = thenableIndexCounter; thenableIndexCounter += 1; @@ -151,157 +137,27 @@ function unwrapThenable(thenable: Thenable): T { return trackUsedThenable(thenableState, thenable, index); } -type CoercedStringRef = ((handle: mixed) => void) & {_stringRef: ?string, ...}; - -function convertStringRefToCallbackRef( - returnFiber: Fiber, - current: Fiber | null, - element: ReactElement, - mixedRef: string | number | boolean, -): CoercedStringRef { - if (__DEV__) { - checkPropStringCoercion(mixedRef, 'ref'); - } - const stringRef = '' + (mixedRef: any); - - const owner: ?Fiber = (element._owner: any); - if (!owner) { - throw new Error( - `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" + - '3. You have multiple copies of React loaded\n' + - 'See https://reactjs.org/link/refs-must-have-owner for more information.', - ); - } - if (owner.tag !== ClassComponent) { - throw new Error( - 'Function components cannot have string refs. ' + - 'We recommend using useRef() instead. ' + - 'Learn more about using refs safely here: ' + - 'https://reactjs.org/link/strict-mode-string-ref', - ); - } - - if (__DEV__) { - if ( - // Will already warn with "Function components cannot be given refs" - !(typeof element.type === 'function' && !isReactClass(element.type)) - ) { - const componentName = - getComponentNameFromFiber(returnFiber) || 'Component'; - if (!didWarnAboutStringRefs[componentName]) { - console.error( - 'Component "%s" contains the string ref "%s". Support for string refs ' + - 'will be removed in a future major release. We recommend using ' + - 'useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: ' + - 'https://reactjs.org/link/strict-mode-string-ref', - componentName, - stringRef, - ); - didWarnAboutStringRefs[componentName] = true; - } - } - } - - const inst = owner.stateNode; - if (!inst) { - throw new Error( - `Missing owner for string ref ${stringRef}. This error is likely caused by a ` + - 'bug in React. Please file an issue.', - ); - } - - // Check if previous string ref matches new string ref - if ( - current !== null && - current.ref !== null && - typeof current.ref === 'function' && - current.ref._stringRef === stringRef - ) { - // Reuse the existing string ref - const currentRef: CoercedStringRef = ((current.ref: any): CoercedStringRef); - return currentRef; - } - - // Create a new string ref - const ref = function (value: mixed) { - const refs = inst.refs; - if (value === null) { - delete refs[stringRef]; - } else { - refs[stringRef] = value; - } - }; - ref._stringRef = stringRef; - return ref; -} - function coerceRef( returnFiber: Fiber, current: Fiber | null, workInProgress: Fiber, element: ReactElement, ): void { - let mixedRef; + let ref; if (enableRefAsProp) { // TODO: This is a temporary, intermediate step. When enableRefAsProp is on, // we should resolve the `ref` prop during the begin phase of the component // it's attached to (HostComponent, ClassComponent, etc). const refProp = element.props.ref; - mixedRef = refProp !== undefined ? refProp : null; + ref = refProp !== undefined ? refProp : null; } else { // Old behavior. - mixedRef = element.ref; - } - - let coercedRef; - if ( - !disableStringRefs && - (typeof mixedRef === 'string' || - typeof mixedRef === 'number' || - typeof mixedRef === 'boolean') - ) { - coercedRef = convertStringRefToCallbackRef( - returnFiber, - current, - element, - mixedRef, - ); - - if (enableRefAsProp) { - // When enableRefAsProp is on, we should always use the props as the - // source of truth for refs. Not a field on the fiber. - // - // In the case of string refs, this presents a problem, because string - // refs are not passed around internally as strings; they are converted to - // callback refs. The ref used by the reconciler is not the same as the - // one the user provided. - // - // But since this is a deprecated feature anyway, what we can do is clone - // the props object and replace it with the internal callback ref. Then we - // can continue to use the props object as the source of truth. - // - // This means the internal callback ref will leak into userspace. The - // receiving component will receive a callback ref even though the parent - // passed a string. Which is weird, but again, this is a deprecated - // feature, and we're only leaving it around behind a flag so that Meta - // can keep using string refs temporarily while they finish migrating - // their codebase. - const userProvidedProps = workInProgress.pendingProps; - const propsWithInternalCallbackRef = assign({}, userProvidedProps); - propsWithInternalCallbackRef.ref = coercedRef; - workInProgress.pendingProps = propsWithInternalCallbackRef; - } - } else { - coercedRef = mixedRef; + ref = element.ref; } // TODO: If enableRefAsProp is on, we shouldn't use the `ref` field. We // should always read the ref from the prop. - workInProgress.ref = coercedRef; + workInProgress.ref = ref; } function throwOnInvalidObjectType(returnFiber: Fiber, newChild: Object) { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index a034de39d6317..46eeda28f86cb 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -1374,6 +1374,7 @@ describe('ReactIncrementalSideEffects', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + + ' in Bar (at **)\n' + ' in Foo (at **)', ]); expect(fooInstance.refs.bar.test).toEqual('test'); diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index 9e187b15979fd..b162a61848918 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -552,7 +552,7 @@ describe 'ReactCoffeeScriptClass', -> 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in Foo (at **)' + ' in _Class (at **)' ]); expect(ref.current.refs.inner.getName()).toBe 'foo' diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 690950f0b3364..ca568350632ed 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -591,7 +591,7 @@ describe('ReactES6Class', () => { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in Foo (at **)', + ' in Inner (at **)', ]); expect(ref.current.refs.inner.getName()).toBe('foo'); }); diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index aec18a3e0fcb9..b3f6470c83170 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -358,16 +358,20 @@ describe('ReactElementClone', () => { const clone = React.cloneElement(element, props); expect(clone.type).toBe(ComponentClass); expect(clone.key).toBe('12'); - if (gate(flags => flags.enableRefAsProp)) { + if (gate(flags => flags.enableRefAsProp && flags.disableStringRefs)) { expect(clone.props.ref).toBe('34'); expect(() => expect(clone.ref).toBe('34')).toErrorDev( 'Accessing element.ref is no longer supported', {withoutStack: true}, ); expect(clone.props).toEqual({foo: 'ef', ref: '34'}); - } else { - expect(clone.ref).toBe('34'); + } else if ( + gate(flags => !flags.enableRefAsProp && !flags.disableStringRefs) + ) { + expect(clone.ref).toBe(element.ref); expect(clone.props).toEqual({foo: 'ef'}); + } else { + // Not going to bother testing every possible combination. } if (__DEV__) { expect(Object.isFrozen(element)).toBe(true); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 32f4682385c88..fb74c5eacacf7 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -995,11 +995,11 @@ describe('string refs', () => { root.render(); }); }).toErrorDev( - 'Warning: Component "StrictMode" contains the string ref "somestring". ' + + 'Warning: Component "OuterComponent" contains the string ref "somestring". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in OuterComponent (at **)', + ' in InnerComponent (at **)', ); await act(() => { @@ -1034,11 +1034,11 @@ describe('string refs', () => { root.render(); }); }).toErrorDev( - 'Warning: Component "StrictMode" contains the string ref "somestring". ' + + 'Warning: Component "OuterComponent" contains the string ref "somestring". ' + 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in OuterComponent (at **)', + ' in InnerComponent (at **)', ); await act(() => { diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index b40fb99829af5..63e9adbfe0e03 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -699,7 +699,7 @@ describe('ReactTypeScriptClass', function() { 'Support for string refs will be removed in a future major release. ' + 'We recommend using useRef() or createRef() instead. ' + 'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' + - ' in ClassicRefs (at **)', + ' in Inner (at **)', ]); expect(ref.current.refs.inner.getName()).toBe('foo'); }); diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index 7ca9a43b159d0..46c4b507add89 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -19,6 +19,9 @@ import isValidElementType from 'shared/isValidElementType'; import isArray from 'shared/isArray'; import {describeUnknownElementTypeFrameInDEV} from 'shared/ReactComponentStackFrame'; import {enableRefAsProp, disableStringRefs} from 'shared/ReactFeatureFlags'; +import {checkPropStringCoercion} from 'shared/CheckStringCoercion'; +import {ClassComponent} from 'react-reconciler/src/ReactWorkTags'; +import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; @@ -325,6 +328,9 @@ export function jsxProd(type, config, maybeKey) { if (hasValidRef(config)) { if (!enableRefAsProp) { ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, ReactCurrentOwner.current, type); + } } } @@ -336,7 +342,15 @@ export function jsxProd(type, config, maybeKey) { propName !== 'key' && (enableRefAsProp || propName !== 'ref') ) { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef( + config[propName], + ReactCurrentOwner.current, + type, + ); + } else { + props[propName] = config[propName]; + } } } @@ -536,6 +550,9 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { if (hasValidRef(config)) { if (!enableRefAsProp) { ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, ReactCurrentOwner.current, type); + } } if (!disableStringRefs) { warnIfStringRefCannotBeAutoConverted(config, self); @@ -550,7 +567,15 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { propName !== 'key' && (enableRefAsProp || propName !== 'ref') ) { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef( + config[propName], + ReactCurrentOwner.current, + type, + ); + } else { + props[propName] = config[propName]; + } } } @@ -666,6 +691,9 @@ export function createElement(type, config, children) { if (hasValidRef(config)) { if (!enableRefAsProp) { ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, ReactCurrentOwner.current, type); + } } if (__DEV__ && !disableStringRefs) { @@ -693,7 +721,15 @@ export function createElement(type, config, children) { propName !== '__self' && propName !== '__source' ) { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef( + config[propName], + ReactCurrentOwner.current, + type, + ); + } else { + props[propName] = config[propName]; + } } } } @@ -842,6 +878,9 @@ export function cloneElement(element, config, children) { if (!enableRefAsProp) { // Silently steal the ref from the parent. ref = config.ref; + if (!disableStringRefs) { + ref = coerceStringRef(ref, owner, element.type); + } } owner = ReactCurrentOwner.current; } @@ -880,7 +919,11 @@ export function cloneElement(element, config, children) { // Resolve default props props[propName] = defaultProps[propName]; } else { - props[propName] = config[propName]; + if (enableRefAsProp && !disableStringRefs && propName === 'ref') { + props.ref = coerceStringRef(config[propName], owner, element.type); + } else { + props[propName] = config[propName]; + } } } } @@ -1100,3 +1143,89 @@ function validateFragmentProps(fragment) { } } } + +function coerceStringRef(mixedRef, owner, type) { + if (disableStringRefs) { + return mixedRef; + } + + let stringRef; + if (typeof mixedRef === 'string') { + stringRef = mixedRef; + } else { + if (typeof mixedRef === 'number' || typeof mixedRef === 'boolean') { + if (__DEV__) { + checkPropStringCoercion(mixedRef, 'ref'); + } + stringRef = '' + mixedRef; + } else { + return mixedRef; + } + } + + return stringRefAsCallbackRef.bind(null, stringRef, type, owner); +} + +function stringRefAsCallbackRef(stringRef, type, owner, value) { + if (disableStringRefs) { + return; + } + if (!owner) { + throw new Error( + `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" + + '3. You have multiple copies of React loaded\n' + + 'See https://reactjs.org/link/refs-must-have-owner for more information.', + ); + } + if (owner.tag !== ClassComponent) { + throw new Error( + 'Function components cannot have string refs. ' + + 'We recommend using useRef() instead. ' + + 'Learn more about using refs safely here: ' + + 'https://reactjs.org/link/strict-mode-string-ref', + ); + } + + if (__DEV__) { + if ( + // Will already warn with "Function components cannot be given refs" + !(typeof type === 'function' && !isReactClass(type)) + ) { + const componentName = getComponentNameFromFiber(owner) || 'Component'; + if (!didWarnAboutStringRefs[componentName]) { + console.error( + 'Component "%s" contains the string ref "%s". Support for string refs ' + + 'will be removed in a future major release. We recommend using ' + + 'useRef() or createRef() instead. ' + + 'Learn more about using refs safely here: ' + + 'https://reactjs.org/link/strict-mode-string-ref', + componentName, + stringRef, + ); + didWarnAboutStringRefs[componentName] = true; + } + } + } + + const inst = owner.stateNode; + if (!inst) { + throw new Error( + `Missing owner for string ref ${stringRef}. This error is likely caused by a ` + + 'bug in React. Please file an issue.', + ); + } + + const refs = inst.refs; + if (value === null) { + delete refs[stringRef]; + } else { + refs[stringRef] = value; + } +} + +function isReactClass(type) { + return type.prototype && type.prototype.isReactComponent; +}