From 8bf8b678629c67603683c95790424a9f2bc16ca1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 20 Feb 2024 18:28:49 -0500 Subject: [PATCH] Remove string refs (behind flag) This removes string refs, which has been deprecated in Strict Mode for seven years. I've left them behind a flag for Meta, but in open source this fully removes the feature. --- .../src/__tests__/ReactComponent-test.js | 2 + .../ReactDOMServerIntegrationRefs-test.js | 1 + .../ReactDeprecationWarnings-test.js | 67 ++++++++++--------- .../__tests__/ReactFunctionComponent-test.js | 1 + .../multiple-copies-of-react-test.js | 1 + packages/react-dom/src/__tests__/refs-test.js | 5 ++ .../react-reconciler/src/ReactChildFiber.js | 3 +- .../src/ReactFiberCommitWork.js | 7 +- .../src/__tests__/ReactFiberRefs-test.js | 22 ++++++ .../ReactIncrementalSideEffects-test.js | 1 + .../ReactCoffeeScriptClass-test.coffee | 37 +++++----- .../react/src/__tests__/ReactES6Class-test.js | 36 +++++----- ...ofilerDevToolsIntegration-test.internal.js | 10 ++- .../src/__tests__/ReactStrictMode-test.js | 2 + .../__tests__/ReactTypeScriptClass-test.ts | 28 ++++---- packages/react/src/jsx/ReactJSXElement.js | 9 ++- 16 files changed, 144 insertions(+), 88 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index aae31c3494626..ffc36ebe862f5 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -38,6 +38,7 @@ describe('ReactComponent', () => { }).toThrowError(/Target container is not a DOM element./); }); + // @gate !disableStringRefs || !__DEV__ it('should throw when supplying a string ref outside of render method', async () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); @@ -125,6 +126,7 @@ describe('ReactComponent', () => { } }); + // @gate !disableStringRefs it('should support string refs on owned components', async () => { const innerObj = {}; const outerObj = {}; diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index 1bef0f6447881..1bb505b589c23 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -76,6 +76,7 @@ describe('ReactDOMServerIntegration', () => { expect(refElement).toBe(e); }); + // @gate !disableStringRefs it('should have string refs on client when rendered over server markup', async () => { class RefsComponent extends React.Component { render() { diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js index 4c6a0bed84a6c..7064520dec5b3 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js @@ -64,6 +64,7 @@ describe('ReactDeprecationWarnings', () => { ); }); + // @gate !disableStringRefs it('should warn when given string refs', async () => { class RefComponent extends React.Component { render() { @@ -87,6 +88,7 @@ describe('ReactDeprecationWarnings', () => { ); }); + // @gate !disableStringRefs it('should warn when owner and self are the same for string refs', async () => { class RefComponent extends React.Component { render() { @@ -109,6 +111,7 @@ describe('ReactDeprecationWarnings', () => { await waitForAll([]); }); + // @gate !disableStringRefs it('should warn when owner and self are different for string refs', async () => { class RefComponent extends React.Component { render() { @@ -139,39 +142,39 @@ describe('ReactDeprecationWarnings', () => { ]); }); - if (__DEV__) { - it('should warn when owner and self are different for string refs', async () => { - class RefComponent extends React.Component { - render() { - return null; - } + // @gate __DEV__ + // @gate !disableStringRefs + it('should warn when owner and self are different for string refs', async () => { + class RefComponent extends React.Component { + render() { + return null; } - class Component extends React.Component { - render() { - return JSXDEVRuntime.jsxDEV( - RefComponent, - {ref: 'refComponent'}, - null, - false, - {}, - {}, - ); - } + } + class Component extends React.Component { + render() { + return JSXDEVRuntime.jsxDEV( + RefComponent, + {ref: 'refComponent'}, + null, + false, + {}, + {}, + ); } + } - ReactNoop.render(); - await expect(async () => await waitForAll([])).toErrorDev([ - 'Warning: Component "Component" contains the string ref "refComponent". ' + - 'Support for string refs will be removed in a future major release. ' + - 'This case cannot be automatically converted to an arrow function. ' + - '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', - ]); - }); - } + ReactNoop.render(); + await expect(async () => await waitForAll([])).toErrorDev([ + 'Warning: Component "Component" contains the string ref "refComponent". ' + + 'Support for string refs will be removed in a future major release. ' + + 'This case cannot be automatically converted to an arrow function. ' + + '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 7729bfd791c0b..e73765257dc88 100644 --- a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js @@ -173,6 +173,7 @@ describe('ReactFunctionComponent', () => { ).resolves.not.toThrowError(); }); + // @gate !disableStringRefs it('should throw on string refs in pure functions', async () => { function Child() { return
; 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 96647eed81cf4..1afb3d35e7cbf 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,6 +22,7 @@ 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); diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index fbc2133237e43..a6b7d7fea76fc 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -164,6 +164,7 @@ describe('reactiverefs', () => { * Ensure that for every click log there is a corresponding ref (from the * perspective of the injected ClickCounter component. */ + // @gate !disableStringRefs it('Should increase refs with an increase in divs', async () => { const testRefsComponent = await renderTestRefsComponent(); const clickIncrementer = @@ -366,6 +367,7 @@ describe('ref swapping', () => { expect(refCalled).toBe(1); }); + // @gate !disableStringRefs it('coerces numbers to strings', async () => { class A extends React.Component { render() { @@ -390,6 +392,7 @@ describe('ref swapping', () => { 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); @@ -547,6 +550,7 @@ describe('creating element with string ref in constructor', () => { } } + // @gate !disableStringRefs it('throws an error', async () => { await expect(async function () { const container = document.createElement('div'); @@ -567,6 +571,7 @@ describe('creating element with string ref in constructor', () => { }); describe('strings refs across renderers', () => { + // @gate !disableStringRefs it('does not break', async () => { class Parent extends React.Component { render() { diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 41cdf9a02f2ff..ac3c175f2acb1 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -44,7 +44,7 @@ import { import isArray from 'shared/isArray'; import assign from 'shared/assign'; import {checkPropStringCoercion} from 'shared/CheckStringCoercion'; -import {enableRefAsProp} from 'shared/ReactFeatureFlags'; +import {enableRefAsProp, disableStringRefs} from 'shared/ReactFeatureFlags'; import { createWorkInProgress, @@ -266,6 +266,7 @@ function coerceRef( let coercedRef; if ( + !disableStringRefs && mixedRef !== null && typeof mixedRef !== 'function' && typeof mixedRef !== 'object' diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 8b44a80e00eaa..9125035a3f8c0 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -54,6 +54,7 @@ import { enableFloat, enableLegacyHidden, alwaysThrottleRetries, + disableStringRefs, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -1624,7 +1625,11 @@ function commitAttachRef(finishedWork: Fiber) { } } else { if (__DEV__) { - if (!ref.hasOwnProperty('current')) { + // TODO: We should move these warnings to happen during the render + // phase (markRef). + if (disableStringRefs && typeof ref === 'string') { + console.error('String refs are no longer supported.'); + } else if (!ref.hasOwnProperty('current')) { console.error( 'Unexpected ref object provided for %s. ' + 'Use either a ref-setter function or React.createRef().', diff --git a/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js b/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js index 3094738b73bb0..b753f9dd0731e 100644 --- a/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js @@ -86,6 +86,7 @@ describe('ReactFiberRefs', () => { }); // @gate enableRefAsProp + // @gate !disableStringRefs test('string ref props are converted to function refs', async () => { let refProp; function Child({ref}) { @@ -112,4 +113,25 @@ describe('ReactFiberRefs', () => { expect(typeof refProp === 'function').toBe(true); expect(owner.refs.child.type).toBe('div'); }); + + // @gate disableStringRefs + test('log an error in dev if a string ref is passed to a ref-receiving component', async () => { + let refProp; + function Child({ref}) { + refProp = ref; + return
; + } + + class Owner extends React.Component { + render() { + return ; + } + } + + const root = ReactNoop.createRoot(); + await expect(async () => { + await expect(act(() => root.render())).rejects.toThrow(); + }).toErrorDev('String refs are no longer supported'); + expect(refProp).toBe('child'); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index 1e34dd9cde5d2..a034de39d6317 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -1346,6 +1346,7 @@ describe('ReactIncrementalSideEffects', () => { // TODO: Test that mounts, updates, refs, unmounts and deletions happen in the // expected way for aborted and resumed render life-cycles. + // @gate !disableStringRefs it('supports string refs', async () => { let fooInstance = null; diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index 3924d579ad82c..9e187b15979fd 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -535,25 +535,26 @@ describe 'ReactCoffeeScriptClass', -> test React.createElement(Foo), 'DIV', 'bar-through-context' - it 'supports string refs', -> - class Foo extends React.Component - render: -> - React.createElement(InnerComponent, - name: 'foo' - ref: 'inner' - ) + if !featureFlags.disableStringRefs + it 'supports string refs', -> + class Foo extends React.Component + render: -> + React.createElement(InnerComponent, + name: 'foo' + ref: 'inner' + ) - ref = React.createRef() - expect(-> - test(React.createElement(Foo, ref: ref), 'DIV', 'foo') - ).toErrorDev([ - 'Warning: Component "Foo" 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 Foo (at **)' - ]); - expect(ref.current.refs.inner.getName()).toBe 'foo' + ref = React.createRef() + expect(-> + test(React.createElement(Foo, ref: ref), 'DIV', 'foo') + ).toErrorDev([ + 'Warning: Component "Foo" 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 Foo (at **)' + ]); + expect(ref.current.refs.inner.getName()).toBe 'foo' it 'supports drilling through to the DOM using findDOMNode', -> ref = React.createRef() diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index b4f76b2d7d002..690950f0b3364 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -576,24 +576,26 @@ describe('ReactES6Class', () => { }); } - it('supports string refs', () => { - class Foo extends React.Component { - render() { - return ; + if (!require('shared/ReactFeatureFlags').disableStringRefs) { + it('supports string refs', () => { + class Foo extends React.Component { + render() { + return ; + } } - } - const ref = React.createRef(); - expect(() => { - test(, 'DIV', 'foo'); - }).toErrorDev([ - 'Warning: Component "Foo" 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 Foo (at **)', - ]); - expect(ref.current.refs.inner.getName()).toBe('foo'); - }); + const ref = React.createRef(); + expect(() => { + test(, 'DIV', 'foo'); + }).toErrorDev([ + 'Warning: Component "Foo" 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 Foo (at **)', + ]); + expect(ref.current.refs.inner.getName()).toBe('foo'); + }); + } it('supports drilling through to the DOM using findDOMNode', () => { const ref = React.createRef(); diff --git a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js index 572abd354da63..20263f9058645 100644 --- a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js +++ b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js @@ -118,13 +118,17 @@ describe('ReactProfiler DevTools integration', () => { Scheduler.unstable_advanceTime(20); + function Throws() { + throw new Error('Oops!'); + } + expect(() => { rendered.update( -
+ -
, + , ); - }).toThrow(); + }).toThrow('Oops!'); Scheduler.unstable_advanceTime(20); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 28dd94ad06ca2..32f4682385c88 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -968,6 +968,7 @@ describe('string refs', () => { act = require('internal-test-utils').act; }); + // @gate !disableStringRefs it('should warn within a strict tree', async () => { const {StrictMode} = React; @@ -1006,6 +1007,7 @@ describe('string refs', () => { }); }); + // @gate !disableStringRefs it('should warn within a strict tree', async () => { const {StrictMode} = React; diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index 9cc9c1cc24b15..b40fb99829af5 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -689,19 +689,21 @@ describe('ReactTypeScriptClass', function() { }); } - it('supports string refs', function() { - const ref = React.createRef(); - expect(() => { - test(React.createElement(ClassicRefs, {ref: ref}), 'DIV', 'foo'); - }).toErrorDev([ - 'Warning: Component "ClassicRefs" 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 ClassicRefs (at **)', - ]); - expect(ref.current.refs.inner.getName()).toBe('foo'); - }); + if (!ReactFeatureFlags.disableStringRefs) { + it('supports string refs', function() { + const ref = React.createRef(); + expect(() => { + test(React.createElement(ClassicRefs, {ref: ref}), 'DIV', 'foo'); + }).toErrorDev([ + 'Warning: Component "ClassicRefs" 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 ClassicRefs (at **)', + ]); + expect(ref.current.refs.inner.getName()).toBe('foo'); + }); + } it('supports drilling through to the DOM using findDOMNode', function() { const ref = React.createRef(); diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js index 7022bfb87e760..7ca9a43b159d0 100644 --- a/packages/react/src/jsx/ReactJSXElement.js +++ b/packages/react/src/jsx/ReactJSXElement.js @@ -18,7 +18,7 @@ import {checkKeyStringCoercion} from 'shared/CheckStringCoercion'; import isValidElementType from 'shared/isValidElementType'; import isArray from 'shared/isArray'; import {describeUnknownElementTypeFrameInDEV} from 'shared/ReactComponentStackFrame'; -import {enableRefAsProp} from 'shared/ReactFeatureFlags'; +import {enableRefAsProp, disableStringRefs} from 'shared/ReactFeatureFlags'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; const ReactDebugCurrentFrame = ReactSharedInternals.ReactDebugCurrentFrame; @@ -62,6 +62,7 @@ function hasValidKey(config) { function warnIfStringRefCannotBeAutoConverted(config, self) { if (__DEV__) { if ( + !disableStringRefs && typeof config.ref === 'string' && ReactCurrentOwner.current && self && @@ -536,7 +537,9 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) { if (!enableRefAsProp) { ref = config.ref; } - warnIfStringRefCannotBeAutoConverted(config, self); + if (!disableStringRefs) { + warnIfStringRefCannotBeAutoConverted(config, self); + } } // Remaining properties are added to a new props object @@ -665,7 +668,7 @@ export function createElement(type, config, children) { ref = config.ref; } - if (__DEV__) { + if (__DEV__ && !disableStringRefs) { warnIfStringRefCannotBeAutoConverted(config, config.__self); } }