Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn on string ref deprecation warning for everybody (not codemoddable) #25383

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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