From 325cb63dd999149228779accbb388fe4c6349751 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 13:48:51 +0200 Subject: [PATCH 1/9] Turn on string ref warnings for everybody --- packages/shared/ReactFeatureFlags.js | 2 +- packages/shared/forks/ReactFeatureFlags.native-fb.js | 2 +- packages/shared/forks/ReactFeatureFlags.native-oss.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.native.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.www.js | 2 +- packages/shared/forks/ReactFeatureFlags.testing.js | 2 +- packages/shared/forks/ReactFeatureFlags.testing.www.js | 2 +- packages/shared/forks/ReactFeatureFlags.www.js | 2 +- 9 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 67a9ac2014d6e..75eda904642b9 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -209,7 +209,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false; // deprecate lat // a deprecated pattern we want to get rid of in the future export const warnAboutSpreadingKeyToJSX = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; // ----------------------------------------------------------------------------- // Debugging and DevTools diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index beafb67bf0716..1826eea498dc9 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -40,7 +40,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 3fdedac1118e1..d9f480c786caf 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -30,7 +30,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index ce7afbcef7084..7823bc145d220 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -30,7 +30,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 7d9054bfeb7de..bae90f8da3b78 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -30,7 +30,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 38278fbc93d42..f218ce46cf2ac 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -30,7 +30,7 @@ export const enableScopeAPI = true; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = true; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 308aaed94dfaa..12035cc8a94c3 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -30,7 +30,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 89b24c8ed6d03..088e6c9a9a803 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -30,7 +30,7 @@ export const enableScopeAPI = true; export const enableCreateEventHandleAPI = true; export const enableSuspenseCallback = true; export const warnAboutDefaultPropsOnFunctionComponents = false; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const disableLegacyContext = __EXPERIMENTAL__; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableTrustedTypesIntegration = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 1811c9bf7ce57..15734262c051a 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -67,7 +67,7 @@ export const enableSchedulingProfiler: boolean = export const enableSchedulerDebugging = true; export const warnAboutDeprecatedLifecycles = true; export const disableLegacyContext = __EXPERIMENTAL__; -export const warnAboutStringRefs = false; +export const warnAboutStringRefs = true; export const warnAboutDefaultPropsOnFunctionComponents = false; export const enableGetInspectorDataForInstanceInProduction = false; From 63c2fe010d9b2ca8a63fe78f35a9450ca6013c2e Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 13:53:28 +0200 Subject: [PATCH 2/9] Enable usage of `string-refs` codemod --- packages/react/src/ReactBaseClasses.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/react/src/ReactBaseClasses.js b/packages/react/src/ReactBaseClasses.js index 23b8f126146dc..4dcdd012da866 100644 --- a/packages/react/src/ReactBaseClasses.js +++ b/packages/react/src/ReactBaseClasses.js @@ -7,9 +7,14 @@ import ReactNoopUpdateQueue from './ReactNoopUpdateQueue'; import assign from 'shared/assign'; +import {warnAboutStringRefs} from 'shared/ReactFeatureFlags'; const emptyObject = {}; -if (__DEV__) { +if ( + __DEV__ && + // For string refs we recommend the `string-refs` codemod that requires unsealed `this.refs` + !warnAboutStringRefs +) { Object.freeze(emptyObject); } From 73b26990410136a7c38cebfd06863c9a78951443 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 13:56:49 +0200 Subject: [PATCH 3/9] Apply `string-refs` codemod ```bash # in react-codemod fork $ ./bin/react-codemod.js --parser flow string-refs $REACT_FORK All done. Results: 1 errors 1876 unmodified 0 skipped 32 ok # in react fork $ yarn prettier-all $ yarn lint --fix --- .../react-14/react-14.test.js | 50 ++++++++++-- .../react-15/react-15.test.js | 50 ++++++++++-- .../react-16/react-16.test.js | 50 ++++++++++-- .../react-17/react-17.test.js | 50 ++++++++++-- .../src/__tests__/ReactComponent-test.js | 29 ++++++- .../__tests__/ReactComponentLifeCycle-test.js | 15 +++- .../__tests__/ReactCompositeComponent-test.js | 65 +++++++++++++--- .../__tests__/ReactDOMEventListener-test.js | 9 ++- .../src/__tests__/ReactDOMInput-test.js | 17 ++++- ...eactDOMServerIntegrationAttributes-test.js | 8 +- .../ReactDOMServerIntegrationRefs-test.js | 8 +- .../ReactDeprecationWarnings-test.internal.js | 26 ++++++- .../__tests__/ReactFunctionComponent-test.js | 15 +++- .../src/__tests__/ReactIdentity-test.js | 7 +- .../ReactServerRenderingHydration-test.js | 6 +- .../src/__tests__/ReactTestUtils-test.js | 21 ++++- .../src/__tests__/ReactUpdates-test.js | 76 ++++++++++++++++--- .../multiple-copies-of-react-test.js | 9 ++- .../src/__tests__/refs-destruction-test.js | 18 ++++- packages/react-dom/src/__tests__/refs-test.js | 51 +++++++++++-- .../ResponderEventPlugin-test.internal.js | 53 +++++++++++-- .../ReactIncrementalSideEffects-test.js | 8 +- .../__tests__/ReactShallowRenderer-test.js | 14 +++- .../ReactShallowRendererMemo-test.js | 14 +++- .../ReactTestRenderer-test.internal.js | 47 ++++++++++-- .../__tests__/ReactContextValidator-test.js | 8 +- .../react/src/__tests__/ReactES6Class-test.js | 9 ++- .../react/src/__tests__/ReactElement-test.js | 6 +- .../src/__tests__/ReactElementClone-test.js | 28 ++++++- .../src/__tests__/ReactJSXElement-test.js | 10 ++- ...ofilerDevToolsIntegration-test.internal.js | 5 +- .../src/__tests__/ReactStrictMode-test.js | 14 +++- 32 files changed, 688 insertions(+), 108 deletions(-) diff --git a/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js b/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js index a7161485a3638..7c497611cc1b7 100644 --- a/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js +++ b/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js @@ -93,7 +93,15 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ; + const element = ( + { + this.refs['34'] = current; + }} + foo="56" + /> + ); expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -535,7 +543,14 @@ xit('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return
; + return ( +
{ + this.refs['inner'] = current; + }} + /> + ); } } const container = document.createElement('div'); @@ -559,9 +574,20 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ; + const inner = ( + { + this.refs['inner'] = current; + }} + /> + ); const outer = ( - + { + this.refs['outer'] = current; + }}> {inner} ); @@ -743,7 +769,11 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- + { + this.refs['childElement'] = current; + }} + />
); } @@ -770,7 +800,15 @@ it('should NOT warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - {() =>
} + + {() => ( +
{ + this.refs['myRef'] = current; + }} + /> + )} + ); } } diff --git a/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js b/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js index 75891a88b8a5f..03bb7eac85510 100644 --- a/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js +++ b/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js @@ -93,7 +93,15 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ; + const element = ( + { + this.refs['34'] = current; + }} + foo="56" + /> + ); expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -530,7 +538,14 @@ xit('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return
; + return ( +
{ + this.refs['inner'] = current; + }} + /> + ); } } const container = document.createElement('div'); @@ -554,9 +569,20 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ; + const inner = ( + { + this.refs['inner'] = current; + }} + /> + ); const outer = ( - + { + this.refs['outer'] = current; + }}> {inner} ); @@ -738,7 +764,11 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- + { + this.refs['childElement'] = current; + }} + />
); } @@ -765,7 +795,15 @@ it('should NOT warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - {() =>
} + + {() => ( +
{ + this.refs['myRef'] = current; + }} + /> + )} + ); } } diff --git a/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js b/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js index 55bcb8ae84ac3..4f0978d812a88 100644 --- a/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js +++ b/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js @@ -93,7 +93,15 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ; + const element = ( + { + this.refs['34'] = current; + }} + foo="56" + /> + ); expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -525,7 +533,14 @@ it('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return
; + return ( +
{ + this.refs['inner'] = current; + }} + /> + ); } } const container = document.createElement('div'); @@ -549,9 +564,20 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ; + const inner = ( + { + this.refs['inner'] = current; + }} + /> + ); const outer = ( - + { + this.refs['outer'] = current; + }}> {inner} ); @@ -729,7 +755,11 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- + { + this.refs['childElement'] = current; + }} + />
); } @@ -752,7 +782,15 @@ it('should warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - {() =>
} + + {() => ( +
{ + this.refs['myRef'] = current; + }} + /> + )} + ); } } diff --git a/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js b/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js index a3687536499cc..d3fde3c441a04 100644 --- a/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js +++ b/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js @@ -93,7 +93,15 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ; + const element = ( + { + this.refs['34'] = current; + }} + foo="56" + /> + ); expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -525,7 +533,14 @@ it('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return
; + return ( +
{ + this.refs['inner'] = current; + }} + /> + ); } } const container = document.createElement('div'); @@ -549,9 +564,20 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ; + const inner = ( + { + this.refs['inner'] = current; + }} + /> + ); const outer = ( - + { + this.refs['outer'] = current; + }}> {inner} ); @@ -729,7 +755,11 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- + { + this.refs['childElement'] = current; + }} + />
); } @@ -752,7 +782,15 @@ it('should warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - {() =>
} + + {() => ( +
{ + this.refs['myRef'] = current; + }} + /> + )} + ); } } diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index effea07a73e7e..b5332ecc0d985 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -37,7 +37,13 @@ describe('ReactComponent', () => { }); it('should throw when supplying a ref outside of render method', () => { - let instance =
; + let instance = ( +
{ + this.refs.badDiv = current; + }} + /> + ); expect(function() { instance = ReactTestUtils.renderIntoDocument(instance); }).toThrow(); @@ -118,9 +124,20 @@ describe('ReactComponent', () => { class Component extends React.Component { render() { - const inner = ; + const inner = ( + { + this.refs.inner = current; + }} + /> + ); const outer = ( - + { + this.refs.outer = current; + }}> {inner} ); @@ -141,7 +158,11 @@ describe('ReactComponent', () => { render() { return ( -
+
{ + this.refs.test = current; + }} + /> ); } diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index 1a02d22d5dbfe..eacec100e4254 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -378,7 +378,14 @@ describe('ReactComponentLifeCycle', () => { } // you would *NEVER* do anything like this in real code! this.state.hasRenderCompleted = true; - return
I am the inner DIV
; + return ( +
{ + this.refs.theDiv = current; + }}> + I am the inner DIV +
+ ); } componentWillUnmount() { @@ -477,7 +484,11 @@ describe('ReactComponentLifeCycle', () => { class Component extends React.Component { render() { return ( - {this.props.tooltipText}
}> + { + this.refs.tooltip = current; + }} + tooltip={
{this.props.tooltipText}
}> {this.props.text} ); diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index d4eec830da3a5..ea74a940a82de 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -79,9 +79,19 @@ describe('ReactCompositeComponent', () => { render() { const toggleActivatedState = this._toggleActivatedState; return !this.state.activated ? ( - + { + this.refs.x = current; + }} + onClick={toggleActivatedState} + /> ) : ( - + { + this.refs.x = current; + }} + onClick={toggleActivatedState} + /> ); } }; @@ -98,7 +108,12 @@ describe('ReactCompositeComponent', () => { render() { const className = this.props.anchorClassOn ? 'anchorClass' : ''; return this.props.renderAnchor ? ( - + { + this.refs.anch = current; + }} + className={className} + /> ) : ( ); @@ -741,8 +756,15 @@ describe('ReactCompositeComponent', () => { class Wrapper extends React.Component { render() { return ( - - + { + this.refs.parent = current; + }}> + { + this.refs.child = current; + }} + /> ); } @@ -1146,10 +1168,18 @@ describe('ReactCompositeComponent', () => { if (this.props.flipped) { return (
- + { + this.refs.static0 = current; + }} + key="B"> B (ignored) - + { + this.refs.static1 = current; + }} + key="A"> A (ignored)
@@ -1157,10 +1187,18 @@ describe('ReactCompositeComponent', () => { } else { return (
- + { + this.refs.static0 = current; + }} + key="A"> A - + { + this.refs.static1 = current; + }} + key="B"> B
@@ -1456,7 +1494,14 @@ describe('ReactCompositeComponent', () => { } render() { - return ; + return ( + { + this.refs.apple = current; + }} + /> + ); } } diff --git a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js index f41ca857946a1..7a961a72e4618 100644 --- a/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMEventListener-test.js @@ -198,7 +198,14 @@ describe('ReactDOMEventListener', () => { }; render() { - const inner =
Inner
; + const inner = ( +
{ + this.refs.inner = current; + }}> + Inner +
+ ); return (
diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index fb2c4bbc380c5..f64e844b2638e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1075,18 +1075,29 @@ describe('ReactDOMInput', () => { return (
{ + this.refs.a = current; + }} type="radio" name="fruit" checked={true} onChange={emptyFunction} /> A - + { + this.refs.b = current; + }} + type="radio" + name="fruit" + onChange={emptyFunction} + /> B
{ + this.refs.c = current; + }} type="radio" name="fruit" defaultChecked={true} diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js index 13b57e08a282d..94951d2630250 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationAttributes-test.js @@ -337,7 +337,13 @@ describe('ReactDOMServerIntegration', () => { itRenders('no ref attribute', async render => { class RefComponent extends React.Component { render() { - return
; + return ( +
{ + this.refs.foo = current; + }} + /> + ); } } const e = await render(); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index 4d39fce80c885..6a6192e88884d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -82,7 +82,13 @@ describe('ReactDOMServerIntegration', () => { it('should have string refs on client when rendered over server markup', async () => { class RefsComponent extends React.Component { render() { - return
; + return ( +
{ + this.refs.myDiv = current; + }} + /> + ); } } diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js index f65c6e85e834b..07454b9d912c2 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js @@ -59,7 +59,13 @@ describe('ReactDeprecationWarnings', () => { } class Component extends React.Component { render() { - return ; + return ( + { + this.refs.refComponent = current; + }} + /> + ); } } @@ -84,7 +90,14 @@ describe('ReactDeprecationWarnings', () => { } class Component extends React.Component { render() { - return ; + return ( + { + this.refs.refComponent = current; + }} + __self={this} + /> + ); } } ReactNoop.renderLegacySyncRoot(); @@ -99,7 +112,14 @@ describe('ReactDeprecationWarnings', () => { } class Component extends React.Component { render() { - return ; + return ( + { + this.refs.refComponent = current; + }} + __self={{}} + /> + ); } } diff --git a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js index 9228316f884ee..7288dd1be41d9 100644 --- a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js @@ -149,7 +149,13 @@ describe('ReactFunctionComponent', () => { it('should throw on string refs in pure functions', () => { function Child() { - return
; + return ( +
{ + this.refs.me = current; + }} + /> + ); } expect(function() { @@ -177,7 +183,12 @@ describe('ReactFunctionComponent', () => { render() { return ( - + { + this.refs.stateless = current; + }} + /> ); } diff --git a/packages/react-dom/src/__tests__/ReactIdentity-test.js b/packages/react-dom/src/__tests__/ReactIdentity-test.js index 97283d8c392db..9aa83082571ca 100644 --- a/packages/react-dom/src/__tests__/ReactIdentity-test.js +++ b/packages/react-dom/src/__tests__/ReactIdentity-test.js @@ -70,7 +70,12 @@ describe('ReactIdentity', () => { render() { return (
- + { + this.refs.span = current; + }} + key={key} + />
); } diff --git a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js index 663cce63c5650..4d57033cf88c8 100644 --- a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js @@ -46,7 +46,11 @@ describe('ReactDOMServerHydration', () => { render() { return ( - + { + this.refs.span = current; + }} + onClick={this.click}> Name: {this.props.name} ); diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 8b1bccac4416a..9066fcd4b1b90 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -224,11 +224,22 @@ describe('ReactTestUtils', () => { class Root extends React.Component { render() { return ( - - + { + this.refs.html = current; + }}> + { + this.refs.head = current; + }}> hello - hello, world + { + this.refs.body = current; + }}> + hello, world + ); } @@ -354,7 +365,9 @@ describe('ReactTestUtils', () => {
{ + this.refs.input = current; + }} onChange={this.props.handleChange} />
diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 4fe596b9b7fb7..4341a92888853 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -155,7 +155,12 @@ describe('ReactUpdates', () => { render() { return (
- + { + this.refs.child = current; + }} + x={this.state.x} + />
); } @@ -208,7 +213,12 @@ describe('ReactUpdates', () => { render() { return (
- + { + this.refs.child = current; + }} + x={this.state.x} + />
); } @@ -342,7 +352,13 @@ describe('ReactUpdates', () => { render() { parentRenderCount++; - return ; + return ( + { + this.refs.child = current; + }} + /> + ); } } @@ -429,14 +445,28 @@ describe('ReactUpdates', () => { class Box extends React.Component { render() { - return
{this.props.children}
; + return ( +
{ + this.refs.boxDiv = current; + }}> + {this.props.children} +
+ ); } } Object.assign(Box.prototype, UpdateLoggingMixin); class Child extends React.Component { render() { - return child; + return ( + { + this.refs.span = current; + }}> + child + + ); } } Object.assign(Child.prototype, UpdateLoggingMixin); @@ -447,9 +477,14 @@ describe('ReactUpdates', () => { const child = this.props.children; return ( - + { + this.refs.box = current; + }}>
{ + this.refs.switcherDiv = current; + }} style={{ display: this.state.tabKey === child.key ? '' : 'none', }}> @@ -464,8 +499,16 @@ describe('ReactUpdates', () => { class App extends React.Component { render() { return ( - - + { + this.refs.switcher = current; + }}> + { + this.refs.child = current; + }} + /> ); } @@ -593,7 +636,12 @@ describe('ReactUpdates', () => { updates.push('Outer-render-' + this.state.x); return (
- + { + this.refs.inner = current; + }} + />
); } @@ -950,7 +998,13 @@ describe('ReactUpdates', () => { }; render() { - return ; + return ( + { + this.refs.child = current; + }} + /> + ); } } 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 5e2d75b657088..504aeb26391b5 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 @@ -16,7 +16,14 @@ class TextWithStringRef extends React.Component { render() { jest.resetModules(); React = require('react'); - return Hello world!; + return ( + { + this.refs.foo = current; + }}> + Hello world! + + ); } } diff --git a/packages/react-dom/src/__tests__/refs-destruction-test.js b/packages/react-dom/src/__tests__/refs-destruction-test.js index 4265599f14f53..4aae7fb2dbb69 100644 --- a/packages/react-dom/src/__tests__/refs-destruction-test.js +++ b/packages/react-dom/src/__tests__/refs-destruction-test.js @@ -43,8 +43,16 @@ describe('refs-destruction', () => { } else { return (
-
- +
{ + this.refs.theInnerDiv = current; + }} + /> + { + this.refs.theInnerClassComponent = current; + }} + />
); } @@ -135,7 +143,11 @@ describe('refs-destruction', () => { render() { return ( - + { + this.refs.ref = current; + }} + /> ); } diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 8e3643d2c75d0..defacdb477290 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -71,11 +71,23 @@ class TestRefsComponent extends React.Component { render() { return (
-
+
{ + this.refs.resetDiv = current; + }} + onClick={this.doReset}> Reset Me By Clicking This.
- - + { + this.refs.myContainer = current; + }}> + { + this.refs.myCounter = current; + }} + initialCount={1} + />
); @@ -162,7 +174,13 @@ if (!require('shared/ReactFeatureFlags').disableModulePatternComponents) { function Comp() { return { render() { - return
; + return ( +
{ + this.refs.elemRef = current; + }} + /> + ); }, }; } @@ -468,7 +486,14 @@ describe('creating element with ref in constructor', () => { class RefTest extends React.Component { constructor(props) { super(props); - this.p =

Hello!

; + this.p = ( +

{ + this.refs.p = current; + }}> + Hello! +

+ ); } render() { @@ -499,8 +524,20 @@ describe('strings refs across renderers', () => { // This component owns both refs. return ( } - child2={
} + child1={ +
{ + this.refs.child1 = current; + }} + /> + } + child2={ +
{ + this.refs.child2 = current; + }} + /> + } /> ); } diff --git a/packages/react-native-renderer/src/__tests__/ResponderEventPlugin-test.internal.js b/packages/react-native-renderer/src/__tests__/ResponderEventPlugin-test.internal.js index c088ee1c6ae61..3813df85bbeaf 100644 --- a/packages/react-native-renderer/src/__tests__/ResponderEventPlugin-test.internal.js +++ b/packages/react-native-renderer/src/__tests__/ResponderEventPlugin-test.internal.js @@ -1387,9 +1387,23 @@ describe('ResponderEventPlugin', () => { class ChildComponent extends React.Component { render() { return ( -
-
-
+
{ + this.refs.DIV = current; + }} + id={this.props.id + '__DIV'}> +
{ + this.refs.DIV_1 = current; + }} + id={this.props.id + '__DIV_1'} + /> +
{ + this.refs.DIV_2 = current; + }} + id={this.props.id + '__DIV_2'} + />
); } @@ -1398,12 +1412,35 @@ describe('ResponderEventPlugin', () => { class ParentComponent extends React.Component { render() { return ( -
-
- - +
{ + this.refs.P = current; + }} + id="P"> +
{ + this.refs.P_P1 = current; + }} + id="P_P1"> + { + this.refs.P_P1_C1 = current; + }} + id="P_P1_C1" + /> + { + this.refs.P_P1_C2 = current; + }} + id="P_P1_C2" + />
-
+
{ + this.refs.P_OneOff = current; + }} + id="P_OneOff" + />
); } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index e09c866d6d67d..c3e250823ecb1 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -1301,7 +1301,13 @@ describe('ReactIncrementalSideEffects', () => { class Foo extends React.Component { render() { fooInstance = this; - return ; + return ( + { + this.refs.bar = current; + }} + /> + ); } } diff --git a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js index c7e7a31acaaf6..585480c5a5891 100644 --- a/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactShallowRenderer-test.js @@ -500,7 +500,13 @@ describe('ReactShallowRenderer', () => { it('can shallow render with a ref', () => { class SomeComponent extends React.Component { render() { - return
; + return ( +
{ + this.refs.hello = current; + }} + /> + ); } } @@ -983,7 +989,11 @@ describe('ReactShallowRenderer', () => { render() { instance = this; return ( - ); diff --git a/packages/react-test-renderer/src/__tests__/ReactShallowRendererMemo-test.js b/packages/react-test-renderer/src/__tests__/ReactShallowRendererMemo-test.js index a0c0f7fc46139..445c2bc2cb170 100644 --- a/packages/react-test-renderer/src/__tests__/ReactShallowRendererMemo-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactShallowRendererMemo-test.js @@ -503,7 +503,13 @@ describe('ReactShallowRendererMemo', () => { it('can shallow render with a ref', () => { class SomeComponent extends React.Component { render() { - return
; + return ( +
{ + this.refs.hello = current; + }} + /> + ); } } @@ -1013,7 +1019,11 @@ describe('ReactShallowRendererMemo', () => { render() { instance = this; return ( - ); diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js index a193e37390220..dd67314ddf3d9 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -272,12 +272,24 @@ describe('ReactTestRenderer', () => { } class Foo extends React.Component { render() { - return ; + return ( + { + this.refs.foo = current; + }} + /> + ); } } class Baz extends React.Component { render() { - return
; + return ( +
{ + this.refs.baz = current; + }} + /> + ); } } ReactTestRenderer.create(); @@ -302,7 +314,14 @@ describe('ReactTestRenderer', () => { log.push(this.refs.bar); } render() { - return Hello, world; + return ( + { + this.refs.bar = current; + }}> + Hello, world + + ); } } function createNodeMock(element) { @@ -355,7 +374,13 @@ describe('ReactTestRenderer', () => { it('supports unmounting when using refs', () => { class Foo extends React.Component { render() { - return
; + return ( +
{ + this.refs.foo = current; + }} + /> + ); } } const inst = ReactTestRenderer.create(, { @@ -394,7 +419,19 @@ describe('ReactTestRenderer', () => { }; class Foo extends React.Component { render() { - return this.props.useDiv ?
: ; + return this.props.useDiv ? ( +
{ + this.refs.foo = current; + }} + /> + ) : ( + { + this.refs.foo = current; + }} + /> + ); } } const inst = ReactTestRenderer.create(, { diff --git a/packages/react/src/__tests__/ReactContextValidator-test.js b/packages/react/src/__tests__/ReactContextValidator-test.js index ae1a9a0da7fe4..7b11a435c780b 100644 --- a/packages/react/src/__tests__/ReactContextValidator-test.js +++ b/packages/react/src/__tests__/ReactContextValidator-test.js @@ -54,7 +54,13 @@ describe('ReactContextValidator', () => { } render() { - return ; + return ( + { + this.refs.child = current; + }} + /> + ); } } ComponentInFooBarContext.childContextTypes = { diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 67b9e11a49805..6b7a8c425515d 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -571,7 +571,14 @@ describe('ReactES6Class', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return ; + return ( + { + this.refs.inner = current; + }} + /> + ); } } const ref = React.createRef(); diff --git a/packages/react/src/__tests__/ReactElement-test.js b/packages/react/src/__tests__/ReactElement-test.js index 0e8a72e2f3888..408bac3978059 100644 --- a/packages/react/src/__tests__/ReactElement-test.js +++ b/packages/react/src/__tests__/ReactElement-test.js @@ -93,7 +93,11 @@ describe('ReactElement', () => { render() { return (
- + { + this.refs.childElement = current; + }} + />
); } diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index 88cbea84cd7eb..4754622191d03 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -83,7 +83,17 @@ describe('ReactElementClone', () => { it('should keep the original ref if it is not overridden', () => { class Grandparent extends React.Component { render() { - return } />; + return ( + { + this.refs.yolo = current; + }} + /> + } + /> + ); } } @@ -188,7 +198,10 @@ describe('ReactElementClone', () => { class Grandparent extends React.Component { render() { return ( - + { + this.refs.parent = current; + }}> ); @@ -210,8 +223,15 @@ describe('ReactElementClone', () => { class Grandparent extends React.Component { render() { return ( - - + { + this.refs.parent = current; + }}> + { + this.refs.child = current; + }} + /> ); } diff --git a/packages/react/src/__tests__/ReactJSXElement-test.js b/packages/react/src/__tests__/ReactJSXElement-test.js index 73aeba559743f..617634065fb7b 100644 --- a/packages/react/src/__tests__/ReactJSXElement-test.js +++ b/packages/react/src/__tests__/ReactJSXElement-test.js @@ -78,7 +78,15 @@ describe('ReactJSXElement', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ; + const element = ( + { + this.refs['34'] = current; + }} + foo="56" + /> + ); expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); diff --git a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js index b6eec52384462..f42179dbcd1dd 100644 --- a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js +++ b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js @@ -114,7 +114,10 @@ describe('ReactProfiler DevTools integration', () => { expect(() => { rendered.update( -
+
{ + this.refs['this-will-cause-an-error'] = current; + }}>
, ); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 0f3ba9a9e1cb9..6e486346e58d3 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -725,7 +725,11 @@ describe('string refs', () => { render() { return ( - + { + this.refs.somestring = current; + }} + /> ); } @@ -768,7 +772,13 @@ describe('string refs', () => { class InnerComponent extends React.Component { render() { - return ; + return ( + { + this.refs.somestring = current; + }} + /> + ); } } From a07749e757b76a7ffe7c971d0da08801f8455afd Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 14:12:22 +0200 Subject: [PATCH 4/9] Adjust tests that were using string refs for something else --- .../react/src/__tests__/ReactElementJSX-test.js | 2 +- .../react/src/__tests__/ReactJSXElement-test.js | 13 +++---------- ...eactProfilerDevToolsIntegration-test.internal.js | 7 +++---- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/packages/react/src/__tests__/ReactElementJSX-test.js b/packages/react/src/__tests__/ReactElementJSX-test.js index 1a4f535a0b2f9..234695aedbecf 100644 --- a/packages/react/src/__tests__/ReactElementJSX-test.js +++ b/packages/react/src/__tests__/ReactElementJSX-test.js @@ -221,7 +221,7 @@ describe('ReactElement.jsx', () => { class Parent extends React.Component { render() { return JSXRuntime.jsx('div', { - children: JSXRuntime.jsx(Child, {ref: 'childElement'}), + children: JSXRuntime.jsx(Child, {ref: React.createRef()}), }); } } diff --git a/packages/react/src/__tests__/ReactJSXElement-test.js b/packages/react/src/__tests__/ReactJSXElement-test.js index 617634065fb7b..563e8be8d69c3 100644 --- a/packages/react/src/__tests__/ReactJSXElement-test.js +++ b/packages/react/src/__tests__/ReactJSXElement-test.js @@ -78,18 +78,11 @@ describe('ReactJSXElement', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ( - { - this.refs['34'] = current; - }} - foo="56" - /> - ); + const ref = React.createRef(); + const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); - expect(element.ref).toBe('34'); + expect(element.ref).toBe(ref); const expectation = {foo: '56'}; Object.freeze(expectation); expect(element.props).toEqual(expectation); diff --git a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js index f42179dbcd1dd..bd8069f311098 100644 --- a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js +++ b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js @@ -101,6 +101,8 @@ describe('ReactProfiler DevTools integration', () => { ); }); + // FIXME: What would throw in a host root apart from string refs without owner? + // @gate !warnAboutStringRefs it('should reset the fiber stack correctly after an error when profiling host roots', () => { Scheduler.unstable_advanceTime(20); @@ -114,10 +116,7 @@ describe('ReactProfiler DevTools integration', () => { expect(() => { rendered.update( -
{ - this.refs['this-will-cause-an-error'] = current; - }}> +
, ); From 49c5d51980e075ac853dd4d7a8781e02d500e55e Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 15:14:07 +0200 Subject: [PATCH 5/9] Ensure class component instances have their own `this.refs` --- .../src/ReactChildFiber.new.js | 32 ++++++++++++++++++ .../src/ReactChildFiber.old.js | 33 +++++++++++++++++++ packages/react/src/ReactBaseClasses.js | 7 +--- 3 files changed, 66 insertions(+), 6 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index a7b2d3e949797..e946c9f0cc41f 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -224,6 +224,38 @@ function coerceRef( } } } + + if (typeof mixedRef === 'function' && element._owner) { + const owner: ?Fiber = (element._owner: any); + if (owner) { + const ownerFiber = ((owner: any): Fiber); + + if (ownerFiber.tag === ClassComponent) { + const functionRef = mixedRef; + const resolvedInst = ownerFiber.stateNode; + + // Check if previous function ref matches new function ref + if ( + current !== null && + current.ref !== null && + typeof current.ref === 'function' && + current.ref._functionRef === functionRef + ) { + return current.ref; + } + + const ref = function(value) { + if (resolvedInst.refs === emptyRefsObject) { + // This is a lazy pooled frozen object, so we need to initialize. + resolvedInst.refs = {}; + } + functionRef.call(resolvedInst, value); + }; + ref._functionRef = functionRef; + return ref; + } + } + } return mixedRef; } diff --git a/packages/react-reconciler/src/ReactChildFiber.old.js b/packages/react-reconciler/src/ReactChildFiber.old.js index 4c880a3e5283d..3ff5ff578f924 100644 --- a/packages/react-reconciler/src/ReactChildFiber.old.js +++ b/packages/react-reconciler/src/ReactChildFiber.old.js @@ -224,6 +224,39 @@ function coerceRef( } } } + + if (typeof mixedRef === 'function' && element._owner) { + const owner: ?Fiber = (element._owner: any); + if (owner) { + const ownerFiber = ((owner: any): Fiber); + + if (ownerFiber.tag === ClassComponent) { + const functionRef = mixedRef; + const resolvedInst = ownerFiber.stateNode; + + // Check if previous function ref matches new function ref + if ( + current !== null && + current.ref !== null && + typeof current.ref === 'function' && + current.ref._functionRef === functionRef + ) { + return current.ref; + } + + const ref = function(value) { + if (resolvedInst.refs === emptyRefsObject) { + // This is a lazy pooled frozen object, so we need to initialize. + resolvedInst.refs = {}; + } + functionRef.call(resolvedInst, value); + }; + ref._functionRef = functionRef; + return ref; + } + } + } + return mixedRef; } diff --git a/packages/react/src/ReactBaseClasses.js b/packages/react/src/ReactBaseClasses.js index 4dcdd012da866..23b8f126146dc 100644 --- a/packages/react/src/ReactBaseClasses.js +++ b/packages/react/src/ReactBaseClasses.js @@ -7,14 +7,9 @@ import ReactNoopUpdateQueue from './ReactNoopUpdateQueue'; import assign from 'shared/assign'; -import {warnAboutStringRefs} from 'shared/ReactFeatureFlags'; const emptyObject = {}; -if ( - __DEV__ && - // For string refs we recommend the `string-refs` codemod that requires unsealed `this.refs` - !warnAboutStringRefs -) { +if (__DEV__) { Object.freeze(emptyObject); } From bebc868ca2d32eb3634c0ea34014dabb3746fa6e Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 15:14:43 +0200 Subject: [PATCH 6/9] Adjust tests that were missed by codemod --- .../ReactMultiChildReconcile-test.js | 4 +- packages/react-dom/src/__tests__/refs-test.js | 77 ++++++++++++------- .../src/__tests__/ReactElementClone-test.js | 13 +++- 3 files changed, 62 insertions(+), 32 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js b/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js index 51cec8b75f718..1a83c8c6816f5 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js @@ -116,7 +116,9 @@ class FriendsStatusDisplay extends React.Component { !status ? null : ( { + this.refs[key] = current; + }} contentKey={key} onFlush={this.verifyPreviousRefsResolved.bind(this, key)} status={status} diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index defacdb477290..8027504d996f0 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -36,7 +36,9 @@ class ClickCounter extends React.Component {
{ + this.refs['clickLog' + i] = current; + }} />, ); } @@ -73,7 +75,11 @@ class TestRefsComponent extends React.Component {
{ - this.refs.resetDiv = current; + if (current === null) { + delete this.refs.resetDiv; + } else { + this.refs.resetDiv = current; + } }} onClick={this.doReset}> Reset Me By Clicking This. @@ -94,15 +100,6 @@ class TestRefsComponent extends React.Component { } } -const expectClickLogsLengthToBe = function(instance, length) { - const clickLogs = ReactTestUtils.scryRenderedDOMComponentsWithClass( - instance, - 'clickLogDiv', - ); - expect(clickLogs.length).toBe(length); - expect(Object.keys(instance.refs.myCounter.refs).length).toBe(length); -}; - describe('reactiverefs', () => { let container; @@ -149,22 +146,47 @@ describe('reactiverefs', () => { 'clickIncrementer', ); - expectClickLogsLengthToBe(testRefsComponent, 1); + let clickLogs = ReactTestUtils.scryRenderedDOMComponentsWithClass( + testRefsComponent, + 'clickLogDiv', + ); + expect(clickLogs.length).toBe(1); + expect(Object.keys(testRefsComponent.refs.myCounter.refs)).toHaveLength(1); // After clicking the reset, there should still only be one click log ref. testRefsComponent.refs.resetDiv.click(); - expectClickLogsLengthToBe(testRefsComponent, 1); + clickLogs = ReactTestUtils.scryRenderedDOMComponentsWithClass( + testRefsComponent, + 'clickLogDiv', + ); + expect(clickLogs.length).toBe(1); + expect(Object.keys(testRefsComponent.refs.myCounter.refs)).toHaveLength(1); // Begin incrementing clicks (and therefore refs). clickIncrementer.click(); - expectClickLogsLengthToBe(testRefsComponent, 2); + clickLogs = ReactTestUtils.scryRenderedDOMComponentsWithClass( + testRefsComponent, + 'clickLogDiv', + ); + expect(clickLogs.length).toBe(2); + expect(Object.keys(testRefsComponent.refs.myCounter.refs)).toHaveLength(2); clickIncrementer.click(); - expectClickLogsLengthToBe(testRefsComponent, 3); + clickLogs = ReactTestUtils.scryRenderedDOMComponentsWithClass( + testRefsComponent, + 'clickLogDiv', + ); + expect(clickLogs.length).toBe(3); + expect(Object.keys(testRefsComponent.refs.myCounter.refs)).toHaveLength(3); // Now reset again testRefsComponent.refs.resetDiv.click(); - expectClickLogsLengthToBe(testRefsComponent, 1); + clickLogs = ReactTestUtils.scryRenderedDOMComponentsWithClass( + testRefsComponent, + 'clickLogDiv', + ); + expect(clickLogs.length).toBe(1); + expect(Object.keys(testRefsComponent.refs.myCounter.refs)).toHaveLength(3); }); }); @@ -230,15 +252,21 @@ describe('ref swapping', () => {
{ + this.refs[count % 3 === 0 ? 'hopRef' : 'divOneRef'] = current; + }} />
{ + this.refs[count % 3 === 1 ? 'hopRef' : 'divTwoRef'] = current; + }} />
{ + this.refs[count % 3 === 2 ? 'hopRef' : 'divThreeRef'] = current; + }} />
); @@ -482,18 +510,11 @@ describe('root level refs', () => { }); }); -describe('creating element with ref in constructor', () => { +describe('creating element with string ref in constructor', () => { class RefTest extends React.Component { constructor(props) { super(props); - this.p = ( -

{ - this.refs.p = current; - }}> - Hello! -

- ); + this.p =

Hello!

; } render() { diff --git a/packages/react/src/__tests__/ReactElementClone-test.js b/packages/react/src/__tests__/ReactElementClone-test.js index 4754622191d03..ad82a08821968 100644 --- a/packages/react/src/__tests__/ReactElementClone-test.js +++ b/packages/react/src/__tests__/ReactElementClone-test.js @@ -185,12 +185,15 @@ describe('ReactElementClone', () => { it('should support keys and refs', () => { class Parent extends React.Component { render() { + const ref = current => { + this.refs.xyz = current; + }; const clone = React.cloneElement(this.props.children, { key: 'xyz', - ref: 'xyz', + ref: ref, }); expect(clone.key).toBe('xyz'); - expect(clone.ref).toBe('xyz'); + expect(clone.ref).toBe(ref); return
{clone}
; } } @@ -215,7 +218,11 @@ describe('ReactElementClone', () => { it('should steal the ref if a new ref is specified', () => { class Parent extends React.Component { render() { - const clone = React.cloneElement(this.props.children, {ref: 'xyz'}); + const clone = React.cloneElement(this.props.children, { + ref: current => { + this.refs.xyz = current; + }, + }); return
{clone}
; } } From ba950fc5ea5f6e078f284a5ff80cd0b8edada1c7 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 15:26:43 +0200 Subject: [PATCH 7/9] Revert codemod where tests explicitly test string refs --- .../react-14/react-14.test.js | 50 +++---------------- .../react-15/react-15.test.js | 50 +++---------------- .../react-16/react-16.test.js | 50 +++---------------- .../react-17/react-17.test.js | 50 +++---------------- .../src/__tests__/ReactComponent-test.js | 33 +++--------- .../ReactDOMServerIntegrationRefs-test.js | 8 +-- .../ReactDeprecationWarnings-test.internal.js | 26 ++-------- .../__tests__/ReactFunctionComponent-test.js | 15 +----- .../multiple-copies-of-react-test.js | 9 +--- .../ReactIncrementalSideEffects-test.js | 8 +-- .../ReactCoffeeScriptClass-test.coffee | 2 +- .../react/src/__tests__/ReactES6Class-test.js | 11 +--- .../src/__tests__/ReactStrictMode-test.js | 14 +----- .../__tests__/ReactTypeScriptClass-test.ts | 2 +- 14 files changed, 44 insertions(+), 284 deletions(-) diff --git a/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js b/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js index 7c497611cc1b7..a7161485a3638 100644 --- a/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js +++ b/fixtures/legacy-jsx-runtimes/react-14/react-14.test.js @@ -93,15 +93,7 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ( - { - this.refs['34'] = current; - }} - foo="56" - /> - ); + const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -543,14 +535,7 @@ xit('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return ( -
{ - this.refs['inner'] = current; - }} - /> - ); + return
; } } const container = document.createElement('div'); @@ -574,20 +559,9 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ( - { - this.refs['inner'] = current; - }} - /> - ); + const inner = ; const outer = ( - { - this.refs['outer'] = current; - }}> + {inner} ); @@ -769,11 +743,7 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- { - this.refs['childElement'] = current; - }} - /> +
); } @@ -800,15 +770,7 @@ it('should NOT warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - - {() => ( -
{ - this.refs['myRef'] = current; - }} - /> - )} - + {() =>
} ); } } diff --git a/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js b/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js index 03bb7eac85510..75891a88b8a5f 100644 --- a/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js +++ b/fixtures/legacy-jsx-runtimes/react-15/react-15.test.js @@ -93,15 +93,7 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ( - { - this.refs['34'] = current; - }} - foo="56" - /> - ); + const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -538,14 +530,7 @@ xit('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return ( -
{ - this.refs['inner'] = current; - }} - /> - ); + return
; } } const container = document.createElement('div'); @@ -569,20 +554,9 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ( - { - this.refs['inner'] = current; - }} - /> - ); + const inner = ; const outer = ( - { - this.refs['outer'] = current; - }}> + {inner} ); @@ -764,11 +738,7 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- { - this.refs['childElement'] = current; - }} - /> +
); } @@ -795,15 +765,7 @@ it('should NOT warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - - {() => ( -
{ - this.refs['myRef'] = current; - }} - /> - )} - + {() =>
} ); } } diff --git a/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js b/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js index 4f0978d812a88..55bcb8ae84ac3 100644 --- a/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js +++ b/fixtures/legacy-jsx-runtimes/react-16/react-16.test.js @@ -93,15 +93,7 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ( - { - this.refs['34'] = current; - }} - foo="56" - /> - ); + const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -533,14 +525,7 @@ it('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return ( -
{ - this.refs['inner'] = current; - }} - /> - ); + return
; } } const container = document.createElement('div'); @@ -564,20 +549,9 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ( - { - this.refs['inner'] = current; - }} - /> - ); + const inner = ; const outer = ( - { - this.refs['outer'] = current; - }}> + {inner} ); @@ -755,11 +729,7 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- { - this.refs['childElement'] = current; - }} - /> +
); } @@ -782,15 +752,7 @@ it('should warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - - {() => ( -
{ - this.refs['myRef'] = current; - }} - /> - )} - + {() =>
} ); } } diff --git a/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js b/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js index d3fde3c441a04..a3687536499cc 100644 --- a/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js +++ b/fixtures/legacy-jsx-runtimes/react-17/react-17.test.js @@ -93,15 +93,7 @@ it('does not reuse the object that is spread into props', () => { }); it('extracts key and ref from the rest of the props', () => { - const element = ( - { - this.refs['34'] = current; - }} - foo="56" - /> - ); + const element = ; expect(element.type).toBe(Component); expect(element.key).toBe('12'); expect(element.ref).toBe('34'); @@ -533,14 +525,7 @@ it('does not call lazy initializers eagerly', () => { it('supports classic refs', () => { class Foo extends React.Component { render() { - return ( -
{ - this.refs['inner'] = current; - }} - /> - ); + return
; } } const container = document.createElement('div'); @@ -564,20 +549,9 @@ it('should support refs on owned components', () => { class Component extends React.Component { render() { - const inner = ( - { - this.refs['inner'] = current; - }} - /> - ); + const inner = ; const outer = ( - { - this.refs['outer'] = current; - }}> + {inner} ); @@ -755,11 +729,7 @@ it('should warn when `ref` is being accessed', () => { render() { return (
- { - this.refs['childElement'] = current; - }} - /> +
); } @@ -782,15 +752,7 @@ it('should warn when owner and self are different for string refs', () => { class ClassParent extends React.Component { render() { return ( - - {() => ( -
{ - this.refs['myRef'] = current; - }} - /> - )} - + {() =>
} ); } } diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index b5332ecc0d985..bb4db5684609e 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -36,14 +36,8 @@ describe('ReactComponent', () => { }).toThrowError(/Target container is not a DOM element./); }); - it('should throw when supplying a ref outside of render method', () => { - let instance = ( -
{ - this.refs.badDiv = current; - }} - /> - ); + it('should throw when supplying a string ref outside of render method', () => { + let instance =
; expect(function() { instance = ReactTestUtils.renderIntoDocument(instance); }).toThrow(); @@ -124,20 +118,9 @@ describe('ReactComponent', () => { class Component extends React.Component { render() { - const inner = ( - { - this.refs.inner = current; - }} - /> - ); + const inner = ; const outer = ( - { - this.refs.outer = current; - }}> + {inner} ); @@ -153,16 +136,12 @@ describe('ReactComponent', () => { ReactTestUtils.renderIntoDocument(); }); - it('should not have refs on unmounted components', () => { + it('should not have string refs on unmounted components', () => { class Parent extends React.Component { render() { return ( -
{ - this.refs.test = current; - }} - /> +
); } diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index 6a6192e88884d..4d39fce80c885 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -82,13 +82,7 @@ describe('ReactDOMServerIntegration', () => { it('should have string refs on client when rendered over server markup', async () => { class RefsComponent extends React.Component { render() { - return ( -
{ - this.refs.myDiv = current; - }} - /> - ); + return
; } } diff --git a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js index 07454b9d912c2..f65c6e85e834b 100644 --- a/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.internal.js @@ -59,13 +59,7 @@ describe('ReactDeprecationWarnings', () => { } class Component extends React.Component { render() { - return ( - { - this.refs.refComponent = current; - }} - /> - ); + return ; } } @@ -90,14 +84,7 @@ describe('ReactDeprecationWarnings', () => { } class Component extends React.Component { render() { - return ( - { - this.refs.refComponent = current; - }} - __self={this} - /> - ); + return ; } } ReactNoop.renderLegacySyncRoot(); @@ -112,14 +99,7 @@ describe('ReactDeprecationWarnings', () => { } class Component extends React.Component { render() { - return ( - { - this.refs.refComponent = current; - }} - __self={{}} - /> - ); + return ; } } diff --git a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js index 7288dd1be41d9..9228316f884ee 100644 --- a/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js @@ -149,13 +149,7 @@ describe('ReactFunctionComponent', () => { it('should throw on string refs in pure functions', () => { function Child() { - return ( -
{ - this.refs.me = current; - }} - /> - ); + return
; } expect(function() { @@ -183,12 +177,7 @@ describe('ReactFunctionComponent', () => { render() { return ( - { - this.refs.stateless = current; - }} - /> + ); } 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 504aeb26391b5..5e2d75b657088 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 @@ -16,14 +16,7 @@ class TextWithStringRef extends React.Component { render() { jest.resetModules(); React = require('react'); - return ( - { - this.refs.foo = current; - }}> - Hello world! - - ); + return Hello world!; } } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index c3e250823ecb1..e09c866d6d67d 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -1301,13 +1301,7 @@ describe('ReactIncrementalSideEffects', () => { class Foo extends React.Component { render() { fooInstance = this; - return ( - { - this.refs.bar = current; - }} - /> - ); + return ; } } diff --git a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee index 772905b7e0142..df950b0d4136d 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -528,7 +528,7 @@ describe 'ReactCoffeeScriptClass', -> test React.createElement(Foo), 'DIV', 'bar-through-context' - it 'supports classic refs', -> + it 'supports string refs', -> class Foo extends React.Component render: -> React.createElement(InnerComponent, diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 6b7a8c425515d..6ccd09759fb12 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -568,17 +568,10 @@ describe('ReactES6Class', () => { test(, 'DIV', 'bar-through-context'); }); - it('supports classic refs', () => { + it('supports string refs', () => { class Foo extends React.Component { render() { - return ( - { - this.refs.inner = current; - }} - /> - ); + return ; } } const ref = React.createRef(); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 6e486346e58d3..0f3ba9a9e1cb9 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -725,11 +725,7 @@ describe('string refs', () => { render() { return ( - { - this.refs.somestring = current; - }} - /> + ); } @@ -772,13 +768,7 @@ describe('string refs', () => { class InnerComponent extends React.Component { render() { - return ( - { - this.refs.somestring = current; - }} - /> - ); + return ; } } diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index 8d53739d338cd..f1d8429e58be3 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -686,7 +686,7 @@ describe('ReactTypeScriptClass', function() { test(React.createElement(ProvideContext), 'DIV', 'bar-through-context'); }); - it('supports classic refs', function() { + it('supports string refs', function() { const ref = React.createRef(); test(React.createElement(ClassicRefs, {ref: ref}), 'DIV', 'foo'); expect(ref.current.refs.inner.getName()).toBe('foo'); From f9fcdc7b2445bd8696c35c437b31f2c14c3fdab1 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 16:34:23 +0200 Subject: [PATCH 8/9] Adjust tests to expect string ref warning --- .../src/__tests__/ReactComponent-test.js | 25 +++++++++++- .../ReactDOMServerIntegrationRefs-test.js | 22 +++++++++-- packages/react-dom/src/__tests__/refs-test.js | 17 +++++++- .../src/ReactChildFiber.new.js | 15 ++++++- .../src/ReactChildFiber.old.js | 16 +++++++- .../ReactIncrementalSideEffects-test.js | 17 +++++++- .../ReactCoffeeScriptClass-test.coffee | 16 +++++++- .../react/src/__tests__/ReactES6Class-test.js | 16 +++++++- ...ofilerDevToolsIntegration-test.internal.js | 2 - .../src/__tests__/ReactStrictMode-test.js | 39 ++++++++++++------- .../__tests__/ReactTypeScriptClass-test.ts | 15 ++++++- 11 files changed, 170 insertions(+), 30 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js index bb4db5684609e..2b202e935b500 100644 --- a/packages/react-dom/src/__tests__/ReactComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactComponent-test.js @@ -12,6 +12,7 @@ let React; let ReactDOM; let ReactDOMServer; +let ReactFeatureFlags; let ReactTestUtils; describe('ReactComponent', () => { @@ -21,6 +22,7 @@ describe('ReactComponent', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMServer = require('react-dom/server'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactTestUtils = require('react-dom/test-utils'); }); @@ -102,7 +104,7 @@ describe('ReactComponent', () => { } }); - it('should support refs on owned components', () => { + it('should support string refs on owned components', () => { const innerObj = {}; const outerObj = {}; @@ -133,7 +135,26 @@ describe('ReactComponent', () => { } } - ReactTestUtils.renderIntoDocument(); + expect(() => { + ReactTestUtils.renderIntoDocument(); + }).toErrorDev( + ReactFeatureFlags.warnAboutStringRefs + ? [ + 'Warning: Component "div" 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 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 **)', + ] + : [], + ); }); it('should not have string refs on unmounted components', () => { diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js index 4d39fce80c885..323371348bfaa 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationRefs-test.js @@ -14,6 +14,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio let React; let ReactDOM; let ReactDOMServer; +let ReactFeatureFlags; let ReactTestUtils; function initModules() { @@ -22,6 +23,7 @@ function initModules() { React = require('react'); ReactDOM = require('react-dom'); ReactDOMServer = require('react-dom/server'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactTestUtils = require('react-dom/test-utils'); // Make them available to the helpers. @@ -91,10 +93,22 @@ describe('ReactDOMServerIntegration', () => { root.innerHTML = markup; let component = null; resetModules(); - await asyncReactDOMRender( - (component = e)} />, - root, - true, + await expect(async () => { + await asyncReactDOMRender( + (component = e)} />, + root, + true, + ); + }).toErrorDev( + ReactFeatureFlags.warnAboutStringRefs + ? [ + 'Warning: Component "RefsComponent" contains the string ref "myDiv". ' + + '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 RefsComponent (at **)', + ] + : [], ); expect(component.refs.myDiv).toBe(root.firstChild); }); diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 8027504d996f0..35a48d6bcb886 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -12,6 +12,7 @@ let React = require('react'); let ReactDOM = require('react-dom'); let ReactTestUtils = require('react-dom/test-utils'); +let ReactFeatureFlags = require('shared/ReactFeatureFlags'); /** * Counts clicks and has a renders an item for each click. Each item rendered @@ -108,6 +109,7 @@ describe('reactiverefs', () => { React = require('react'); ReactDOM = require('react-dom'); ReactTestUtils = require('react-dom/test-utils'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); }); afterEach(() => { @@ -355,7 +357,20 @@ describe('ref swapping', () => { return
; } } - const a = ReactTestUtils.renderIntoDocument(); + let a; + expect(() => { + a = ReactTestUtils.renderIntoDocument(); + }).toErrorDev( + ReactFeatureFlags.warnAboutStringRefs + ? [ + 'Warning: Component "A" contains the string ref "1". ' + + '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 A (at **)', + ] + : [], + ); expect(a.refs[1].nodeName).toBe('DIV'); }); diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index e946c9f0cc41f..f015c3c141354 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -98,6 +98,10 @@ if (__DEV__) { }; } +function isReactClass(type) { + return type.prototype && type.prototype.isReactComponent; +} + function coerceRef( returnFiber: Fiber, current: Fiber | null, @@ -121,7 +125,16 @@ function coerceRef( element._owner && element._self && element._owner.stateNode !== element._self - ) + ) && + // Will already throw with "Function components cannot have string refs" + !( + element._owner && + ((element._owner: any): Fiber).tag !== ClassComponent + ) && + // Will already warn with "Function components cannot be given refs" + !(typeof element.type === 'function' && !isReactClass(element.type)) && + // Will already throw with "Element ref was specified as a string (someStringRef) but no owner was set" + element._owner ) { const componentName = getComponentNameFromFiber(returnFiber) || 'Component'; diff --git a/packages/react-reconciler/src/ReactChildFiber.old.js b/packages/react-reconciler/src/ReactChildFiber.old.js index 3ff5ff578f924..27bf0ac993bc2 100644 --- a/packages/react-reconciler/src/ReactChildFiber.old.js +++ b/packages/react-reconciler/src/ReactChildFiber.old.js @@ -98,6 +98,10 @@ if (__DEV__) { }; } +function isReactClass(type) { + return type.prototype && type.prototype.isReactComponent; +} + function coerceRef( returnFiber: Fiber, current: Fiber | null, @@ -121,7 +125,16 @@ function coerceRef( element._owner && element._self && element._owner.stateNode !== element._self - ) + ) && + // Will already throw with "Function components cannot have string refs" + !( + element._owner && + ((element._owner: any): Fiber).tag !== ClassComponent + ) && + // Will already warn with "Function components cannot be given refs" + !(typeof element.type === 'function' && !isReactClass(element.type)) && + // Will already throw with "Element ref was specified as a string (someStringRef) but no owner was set" + element._owner ) { const componentName = getComponentNameFromFiber(returnFiber) || 'Component'; @@ -256,7 +269,6 @@ function coerceRef( } } } - return mixedRef; } diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js index e09c866d6d67d..712c0e7d07fe9 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalSideEffects-test.js @@ -11,6 +11,7 @@ 'use strict'; let React; +let ReactFeatureFlags; let ReactNoop; let Scheduler; @@ -19,6 +20,7 @@ describe('ReactIncrementalSideEffects', () => { jest.resetModules(); React = require('react'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); }); @@ -1306,8 +1308,19 @@ describe('ReactIncrementalSideEffects', () => { } ReactNoop.render(); - expect(Scheduler).toFlushWithoutYielding(); - + expect(() => { + expect(Scheduler).toFlushWithoutYielding(); + }).toErrorDev( + ReactFeatureFlags.warnAboutStringRefs + ? [ + 'Warning: Component "Foo" contains the string ref "bar". ' + + '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(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 df950b0d4136d..872787214cbca 100644 --- a/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee +++ b/packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee @@ -9,6 +9,7 @@ PropTypes = null React = null ReactDOM = null ReactDOMClient = null +ReactFeatureFlags = null act = null describe 'ReactCoffeeScriptClass', -> @@ -22,6 +23,7 @@ describe 'ReactCoffeeScriptClass', -> React = require 'react' ReactDOM = require 'react-dom' ReactDOMClient = require 'react-dom/client' + ReactFeatureFlags = require 'shared/ReactFeatureFlags' act = require('jest-react').act PropTypes = require 'prop-types' container = document.createElement 'div' @@ -537,7 +539,19 @@ describe 'ReactCoffeeScriptClass', -> ) ref = React.createRef() - test(React.createElement(Foo, ref: ref), 'DIV', 'foo') + expect(-> + test(React.createElement(Foo, ref: ref), 'DIV', 'foo') + ).toErrorDev( + if ReactFeatureFlags.warnAboutStringRefs + then [ + '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 **)' + ] + else [] + ); expect(ref.current.refs.inner.getName()).toBe 'foo' it 'supports drilling through to the DOM using findDOMNode', -> diff --git a/packages/react/src/__tests__/ReactES6Class-test.js b/packages/react/src/__tests__/ReactES6Class-test.js index 6ccd09759fb12..013c26ac85dbd 100644 --- a/packages/react/src/__tests__/ReactES6Class-test.js +++ b/packages/react/src/__tests__/ReactES6Class-test.js @@ -13,6 +13,7 @@ let PropTypes; let React; let ReactDOM; let ReactDOMClient; +let ReactFeatureFlags; let act; describe('ReactES6Class', () => { @@ -31,6 +32,7 @@ describe('ReactES6Class', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); act = require('jest-react').act; container = document.createElement('div'); root = ReactDOMClient.createRoot(container); @@ -575,7 +577,19 @@ describe('ReactES6Class', () => { } } const ref = React.createRef(); - test(, 'DIV', 'foo'); + expect(() => { + test(, 'DIV', 'foo'); + }).toErrorDev( + ReactFeatureFlags.warnAboutStringRefs + ? [ + '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'); }); diff --git a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js index bd8069f311098..b6eec52384462 100644 --- a/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js +++ b/packages/react/src/__tests__/ReactProfilerDevToolsIntegration-test.internal.js @@ -101,8 +101,6 @@ describe('ReactProfiler DevTools integration', () => { ); }); - // FIXME: What would throw in a host root apart from string refs without owner? - // @gate !warnAboutStringRefs it('should reset the fiber stack correctly after an error when profiling host roots', () => { Scheduler.unstable_advanceTime(20); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.js b/packages/react/src/__tests__/ReactStrictMode-test.js index 0f3ba9a9e1cb9..2dca3d95fbbb0 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.js @@ -741,12 +741,18 @@ describe('string refs', () => { expect(() => { ReactDOM.render(, container); }).toErrorDev( - 'Warning: A string ref, "somestring", has been found within a strict mode tree. ' + - 'String refs are a source of potential bugs and should be avoided. ' + - '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 **)', + ReactFeatureFlags.warnAboutStringRefs + ? 'Warning: Component "StrictMode" 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 **)' + : 'Warning: A string ref, "somestring", has been found within a strict mode tree. ' + + 'String refs are a source of potential bugs and should be avoided. ' + + '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 **)', ); // Dedup @@ -782,13 +788,20 @@ describe('string refs', () => { expect(() => { ReactDOM.render(, container); }).toErrorDev( - 'Warning: A string ref, "somestring", has been found within a strict mode tree. ' + - 'String refs are a source of potential bugs and should be avoided. ' + - 'We recommend using useRef() or createRef() instead. ' + - 'Learn more about using refs safely here: ' + - 'https://reactjs.org/link/strict-mode-string-ref\n' + - ' in InnerComponent (at **)\n' + - ' in OuterComponent (at **)', + ReactFeatureFlags.warnAboutStringRefs + ? 'Warning: Component "InnerComponent" 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 InnerComponent (at **)\n' + + ' in OuterComponent (at **)' + : 'Warning: A string ref, "somestring", has been found within a strict mode tree. ' + + 'String refs are a source of potential bugs and should be avoided. ' + + 'We recommend using useRef() or createRef() instead. ' + + 'Learn more about using refs safely here: ' + + 'https://reactjs.org/link/strict-mode-string-ref\n' + + ' in InnerComponent (at **)\n' + + ' in OuterComponent (at **)', ); // Dedup diff --git a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts index f1d8429e58be3..32835f8014eea 100644 --- a/packages/react/src/__tests__/ReactTypeScriptClass-test.ts +++ b/packages/react/src/__tests__/ReactTypeScriptClass-test.ts @@ -17,6 +17,7 @@ import ReactDOMClient = require('react-dom/client'); import ReactDOMTestUtils = require('react-dom/test-utils'); import PropTypes = require('prop-types'); import internalAct = require('jest-react'); +import ReactFeatureFlags = require('shared/ReactFeatureFlags') // Before Each @@ -688,7 +689,19 @@ describe('ReactTypeScriptClass', function() { it('supports string refs', function() { const ref = React.createRef(); - test(React.createElement(ClassicRefs, {ref: ref}), 'DIV', 'foo'); + expect(() => { + test(React.createElement(ClassicRefs, {ref: ref}), 'DIV', 'foo'); + }).toErrorDev( + ReactFeatureFlags.warnAboutStringRefs + ? [ + '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'); }); From e19c22cd392dd9183447432045fd3271181511cb Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 27 Sep 2022 17:05:33 +0200 Subject: [PATCH 9/9] Fix flow --- packages/react-reconciler/src/ReactInternalTypes.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js index 2ee501d2a433f..84807fbcfa082 100644 --- a/packages/react-reconciler/src/ReactInternalTypes.js +++ b/packages/react-reconciler/src/ReactInternalTypes.js @@ -128,7 +128,11 @@ export type Fiber = { // I'll avoid adding an owner field for prod and model that as functions. ref: | null - | (((handle: mixed) => void) & {_stringRef: ?string, ...}) + | (((handle: mixed) => void) & { + _stringRef?: string, + _functionRef?: (handle: mixed) => void, + ... + }) | RefObject, // Input is the data coming into process this fiber. Arguments. Props.