Skip to content

Commit

Permalink
Turn on string ref deprecation warning for everybody (not codemoddabl…
Browse files Browse the repository at this point in the history
…e) (#25383)

## Summary
 
Alternate to #25334 without any
prod runtime changes i.e. the proposed codemod in
https://github.com/reactjs/rfcs/blob/createlement-rfc/text/0000-create-element-changes.md#deprecate-string-refs-and-remove-production-mode-_owner-field
would not work.

## How did you test this change?

- [x] CI
- [x] `yarn test` with and without `warnAboutStringRefs`
  • Loading branch information
eps1lon authored Nov 17, 2022
1 parent 07f46ec commit 6fb8133
Show file tree
Hide file tree
Showing 37 changed files with 591 additions and 300 deletions.
29 changes: 25 additions & 4 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
let React;
let ReactDOM;
let ReactDOMServer;
let ReactFeatureFlags;
let ReactTestUtils;

describe('ReactComponent', () => {
Expand All @@ -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');
});

Expand All @@ -36,7 +38,7 @@ describe('ReactComponent', () => {
}).toThrowError(/Target container is not a DOM element./);
});

it('should throw when supplying a ref outside of render method', () => {
it('should throw when supplying a string ref outside of render method', () => {
let instance = <div ref="badDiv" />;
expect(function() {
instance = ReactTestUtils.renderIntoDocument(instance);
Expand Down Expand Up @@ -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 = {};

Expand Down Expand Up @@ -133,10 +135,29 @@ describe('ReactComponent', () => {
}
}

ReactTestUtils.renderIntoDocument(<Component />);
expect(() => {
ReactTestUtils.renderIntoDocument(<Component />);
}).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 refs on unmounted components', () => {
it('should not have string refs on unmounted components', () => {
class Parent extends React.Component {
render() {
return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ describe('ReactComponentLifeCycle', () => {
}
// you would *NEVER* do anything like this in real code!
this.state.hasRenderCompleted = true;
return <div ref="theDiv">I am the inner DIV</div>;
return <div ref={React.createRef()}>I am the inner DIV</div>;
}

componentWillUnmount() {
Expand Down Expand Up @@ -477,7 +477,9 @@ describe('ReactComponentLifeCycle', () => {
class Component extends React.Component {
render() {
return (
<Tooltip ref="tooltip" tooltip={<div>{this.props.tooltipText}</div>}>
<Tooltip
ref={React.createRef()}
tooltip={<div>{this.props.tooltipText}</div>}>
{this.props.text}
</Tooltip>
);
Expand Down
63 changes: 37 additions & 26 deletions packages/react-dom/src/__tests__/ReactCompositeComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,18 @@ describe('ReactCompositeComponent', () => {
MorphingComponent = class extends React.Component {
state = {activated: false};

xRef = React.createRef();

_toggleActivatedState = () => {
this.setState({activated: !this.state.activated});
};

render() {
const toggleActivatedState = this._toggleActivatedState;
return !this.state.activated ? (
<a ref="x" onClick={toggleActivatedState} />
<a ref={this.xRef} onClick={toggleActivatedState} />
) : (
<b ref="x" onClick={toggleActivatedState} />
<b ref={this.xRef} onClick={toggleActivatedState} />
);
}
};
Expand All @@ -91,14 +93,16 @@ describe('ReactCompositeComponent', () => {
* reallocated again.
*/
ChildUpdates = class extends React.Component {
anchorRef = React.createRef();

getAnchor = () => {
return this.refs.anch;
return this.anchorRef.current;
};

render() {
const className = this.props.anchorClassOn ? 'anchorClass' : '';
return this.props.renderAnchor ? (
<a ref="anch" className={className} />
<a ref={this.anchorRef} className={className} />
) : (
<b />
);
Expand Down Expand Up @@ -186,11 +190,11 @@ describe('ReactCompositeComponent', () => {
it('should rewire refs when rendering to different child types', () => {
const instance = ReactTestUtils.renderIntoDocument(<MorphingComponent />);

expect(instance.refs.x.tagName).toBe('A');
expect(instance.xRef.current.tagName).toBe('A');
instance._toggleActivatedState();
expect(instance.refs.x.tagName).toBe('B');
expect(instance.xRef.current.tagName).toBe('B');
instance._toggleActivatedState();
expect(instance.refs.x.tagName).toBe('A');
expect(instance.xRef.current.tagName).toBe('A');
});

it('should not cache old DOM nodes when switching constructors', () => {
Expand Down Expand Up @@ -739,25 +743,28 @@ describe('ReactCompositeComponent', () => {
}

class Wrapper extends React.Component {
parentRef = React.createRef();
childRef = React.createRef();

render() {
return (
<Parent ref="parent">
<Child ref="child" />
<Parent ref={this.parentRef}>
<Child ref={this.childRef} />
</Parent>
);
}
}

const wrapper = ReactTestUtils.renderIntoDocument(<Wrapper />);

expect(wrapper.refs.parent.state.flag).toEqual(true);
expect(wrapper.refs.child.context).toEqual({flag: true});
expect(wrapper.parentRef.current.state.flag).toEqual(true);
expect(wrapper.childRef.current.context).toEqual({flag: true});

// We update <Parent /> while <Child /> is still a static prop relative to this update
wrapper.refs.parent.setState({flag: false});
wrapper.parentRef.current.setState({flag: false});

expect(wrapper.refs.parent.state.flag).toEqual(false);
expect(wrapper.refs.child.context).toEqual({flag: false});
expect(wrapper.parentRef.current.state.flag).toEqual(false);
expect(wrapper.childRef.current.context).toEqual({flag: false});
});

it('should pass context transitively', () => {
Expand Down Expand Up @@ -1142,25 +1149,28 @@ describe('ReactCompositeComponent', () => {
}

class Component extends React.Component {
static0Ref = React.createRef();
static1Ref = React.createRef();

render() {
if (this.props.flipped) {
return (
<div>
<Static ref="static0" key="B">
<Static ref={this.static0Ref} key="B">
B (ignored)
</Static>
<Static ref="static1" key="A">
<Static ref={this.static1Ref} key="A">
A (ignored)
</Static>
</div>
);
} else {
return (
<div>
<Static ref="static0" key="A">
<Static ref={this.static0Ref} key="A">
A
</Static>
<Static ref="static1" key="B">
<Static ref={this.static1Ref} key="B">
B
</Static>
</div>
Expand All @@ -1171,14 +1181,14 @@ describe('ReactCompositeComponent', () => {

const container = document.createElement('div');
const comp = ReactDOM.render(<Component flipped={false} />, container);
expect(ReactDOM.findDOMNode(comp.refs.static0).textContent).toBe('A');
expect(ReactDOM.findDOMNode(comp.refs.static1).textContent).toBe('B');
expect(ReactDOM.findDOMNode(comp.static0Ref.current).textContent).toBe('A');
expect(ReactDOM.findDOMNode(comp.static1Ref.current).textContent).toBe('B');

// When flipping the order, the refs should update even though the actual
// contents do not
ReactDOM.render(<Component flipped={true} />, container);
expect(ReactDOM.findDOMNode(comp.refs.static0).textContent).toBe('B');
expect(ReactDOM.findDOMNode(comp.refs.static1).textContent).toBe('A');
expect(ReactDOM.findDOMNode(comp.static0Ref.current).textContent).toBe('B');
expect(ReactDOM.findDOMNode(comp.static1Ref.current).textContent).toBe('A');
});

it('should allow access to findDOMNode in componentWillUnmount', () => {
Expand Down Expand Up @@ -1453,10 +1463,11 @@ describe('ReactCompositeComponent', () => {
this.state = {
color: 'green',
};
this.appleRef = React.createRef();
}

render() {
return <Apple color={this.state.color} ref="apple" />;
return <Apple color={this.state.color} ref={this.appleRef} />;
}
}

Expand Down Expand Up @@ -1502,15 +1513,15 @@ describe('ReactCompositeComponent', () => {
expect(renderCalls).toBe(2);

// Re-render base on state
instance.refs.apple.cut();
instance.appleRef.current.cut();
expect(renderCalls).toBe(3);

// No re-render based on state
instance.refs.apple.cut();
instance.appleRef.current.cut();
expect(renderCalls).toBe(3);

// Re-render based on state again
instance.refs.apple.eatSlice();
instance.appleRef.current.eatSlice();
expect(renderCalls).toBe(4);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,12 +193,13 @@ describe('ReactDOMEventListener', () => {
const onMouseOut = event => mouseOut(event.target);

class Wrapper extends React.Component {
innerRef = React.createRef();
getInner = () => {
return this.refs.inner;
return this.innerRef.current;
};

render() {
const inner = <div ref="inner">Inner</div>;
const inner = <div ref={this.innerRef}>Inner</div>;
return (
<div>
<div onMouseOut={onMouseOut} id="outer">
Expand Down
21 changes: 15 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMInput-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1071,22 +1071,31 @@ describe('ReactDOMInput', () => {

it('should control radio buttons', () => {
class RadioGroup extends React.Component {
aRef = React.createRef();
bRef = React.createRef();
cRef = React.createRef();

render() {
return (
<div>
<input
ref="a"
ref={this.aRef}
type="radio"
name="fruit"
checked={true}
onChange={emptyFunction}
/>
A
<input ref="b" type="radio" name="fruit" onChange={emptyFunction} />
<input
ref={this.bRef}
type="radio"
name="fruit"
onChange={emptyFunction}
/>
B
<form>
<input
ref="c"
ref={this.cRef}
type="radio"
name="fruit"
defaultChecked={true}
Expand All @@ -1099,9 +1108,9 @@ describe('ReactDOMInput', () => {
}

const stub = ReactDOM.render(<RadioGroup />, container);
const aNode = stub.refs.a;
const bNode = stub.refs.b;
const cNode = stub.refs.c;
const aNode = stub.aRef.current;
const bNode = stub.bRef.current;
const cNode = stub.cRef.current;

expect(aNode.checked).toBe(true);
expect(bNode.checked).toBe(false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ describe('ReactDOMServerIntegration', () => {
itRenders('no ref attribute', async render => {
class RefComponent extends React.Component {
render() {
return <div ref="foo" />;
return <div ref={React.createRef()} />;
}
}
const e = await render(<RefComponent />);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const ReactDOMServerIntegrationUtils = require('./utils/ReactDOMServerIntegratio
let React;
let ReactDOM;
let ReactDOMServer;
let ReactFeatureFlags;
let ReactTestUtils;

function initModules() {
Expand All @@ -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.
Expand Down Expand Up @@ -91,10 +93,22 @@ describe('ReactDOMServerIntegration', () => {
root.innerHTML = markup;
let component = null;
resetModules();
await asyncReactDOMRender(
<RefsComponent ref={e => (component = e)} />,
root,
true,
await expect(async () => {
await asyncReactDOMRender(
<RefsComponent ref={e => (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);
});
Expand Down
5 changes: 3 additions & 2 deletions packages/react-dom/src/__tests__/ReactIdentity-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,18 @@ describe('ReactIdentity', () => {

function renderAComponentWithKeyIntoContainer(key, container) {
class Wrapper extends React.Component {
spanRef = React.createRef();
render() {
return (
<div>
<span ref="span" key={key} />
<span ref={this.spanRef} key={key} />
</div>
);
}
}

const instance = ReactDOM.render(<Wrapper />, container);
const span = instance.refs.span;
const span = instance.spanRef.current;
expect(span).not.toBe(null);
}

Expand Down
Loading

0 comments on commit 6fb8133

Please sign in to comment.