Skip to content

Commit

Permalink
Remove string refs (behind flag) (#28322)
Browse files Browse the repository at this point in the history
Depends on:

- #28398

---

This removes string refs, which has been deprecated in Strict Mode for
seven years.

I've left them behind a flag for Meta, but in open source this fully
removes the feature.
  • Loading branch information
acdlite committed Feb 27, 2024
1 parent 3bcd2de commit c979895
Show file tree
Hide file tree
Showing 23 changed files with 154 additions and 88 deletions.
2 changes: 2 additions & 0 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe('ReactComponent', () => {
}).toThrowError(/Target container is not a DOM element./);
});

// @gate !disableStringRefs || !__DEV__
it('should throw when supplying a string ref outside of render method', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
Expand Down Expand Up @@ -125,6 +126,7 @@ describe('ReactComponent', () => {
}
});

// @gate !disableStringRefs
it('should support string refs on owned components', async () => {
const innerObj = {};
const outerObj = {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ describe('ReactDOMServerIntegration', () => {
expect(refElement).toBe(e);
});

// @gate !disableStringRefs
it('should have string refs on client when rendered over server markup', async () => {
class RefsComponent extends React.Component {
render() {
Expand Down
67 changes: 35 additions & 32 deletions packages/react-dom/src/__tests__/ReactDeprecationWarnings-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ describe('ReactDeprecationWarnings', () => {
);
});

// @gate !disableStringRefs
it('should warn when given string refs', async () => {
class RefComponent extends React.Component {
render() {
Expand All @@ -87,6 +88,7 @@ describe('ReactDeprecationWarnings', () => {
);
});

// @gate !disableStringRefs
it('should warn when owner and self are the same for string refs', async () => {
class RefComponent extends React.Component {
render() {
Expand All @@ -109,6 +111,7 @@ describe('ReactDeprecationWarnings', () => {
await waitForAll([]);
});

// @gate !disableStringRefs
it('should warn when owner and self are different for string refs', async () => {
class RefComponent extends React.Component {
render() {
Expand Down Expand Up @@ -139,39 +142,39 @@ describe('ReactDeprecationWarnings', () => {
]);
});

if (__DEV__) {
it('should warn when owner and self are different for string refs', async () => {
class RefComponent extends React.Component {
render() {
return null;
}
// @gate __DEV__
// @gate !disableStringRefs
it('should warn when owner and self are different for string refs', async () => {
class RefComponent extends React.Component {
render() {
return null;
}
class Component extends React.Component {
render() {
return JSXDEVRuntime.jsxDEV(
RefComponent,
{ref: 'refComponent'},
null,
false,
{},
{},
);
}
}
class Component extends React.Component {
render() {
return JSXDEVRuntime.jsxDEV(
RefComponent,
{ref: 'refComponent'},
null,
false,
{},
{},
);
}
}

ReactNoop.render(<Component />);
await expect(async () => await waitForAll([])).toErrorDev([
'Warning: Component "Component" contains the string ref "refComponent". ' +
'Support for string refs will be removed in a future major release. ' +
'This case cannot be automatically converted to an arrow function. ' +
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: ' +
'https://reactjs.org/link/strict-mode-string-ref',
'Warning: Component "Component" contains the string ref "refComponent". ' +
'Support for string refs will be removed in a future major release. We recommend ' +
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
'https://reactjs.org/link/strict-mode-string-ref',
]);
});
}
ReactNoop.render(<Component />);
await expect(async () => await waitForAll([])).toErrorDev([
'Warning: Component "Component" contains the string ref "refComponent". ' +
'Support for string refs will be removed in a future major release. ' +
'This case cannot be automatically converted to an arrow function. ' +
'We ask you to manually fix this case by using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: ' +
'https://reactjs.org/link/strict-mode-string-ref',
'Warning: Component "Component" contains the string ref "refComponent". ' +
'Support for string refs will be removed in a future major release. We recommend ' +
'using useRef() or createRef() instead. Learn more about using refs safely here: ' +
'https://reactjs.org/link/strict-mode-string-ref',
]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ describe('ReactFunctionComponent', () => {
).resolves.not.toThrowError();
});

// @gate !disableStringRefs
it('should throw on string refs in pure functions', async () => {
function Child() {
return <div ref="me" />;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class TextWithStringRef extends React.Component {
}

describe('when different React version is used with string ref', () => {
// @gate !disableStringRefs
it('throws the "Refs must have owner" warning', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
Expand Down
5 changes: 5 additions & 0 deletions packages/react-dom/src/__tests__/refs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ describe('reactiverefs', () => {
* Ensure that for every click log there is a corresponding ref (from the
* perspective of the injected ClickCounter component.
*/
// @gate !disableStringRefs
it('Should increase refs with an increase in divs', async () => {
const testRefsComponent = await renderTestRefsComponent();
const clickIncrementer =
Expand Down Expand Up @@ -366,6 +367,7 @@ describe('ref swapping', () => {
expect(refCalled).toBe(1);
});

// @gate !disableStringRefs
it('coerces numbers to strings', async () => {
class A extends React.Component {
render() {
Expand All @@ -390,6 +392,7 @@ describe('ref swapping', () => {
expect(a.refs[1].nodeName).toBe('DIV');
});

// @gate !disableStringRefs
it('provides an error for invalid refs', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
Expand Down Expand Up @@ -547,6 +550,7 @@ describe('creating element with string ref in constructor', () => {
}
}

// @gate !disableStringRefs
it('throws an error', async () => {
await expect(async function () {
const container = document.createElement('div');
Expand All @@ -567,6 +571,7 @@ describe('creating element with string ref in constructor', () => {
});

describe('strings refs across renderers', () => {
// @gate !disableStringRefs
it('does not break', async () => {
class Parent extends React.Component {
render() {
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactChildFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import {
import isArray from 'shared/isArray';
import assign from 'shared/assign';
import {checkPropStringCoercion} from 'shared/CheckStringCoercion';
import {enableRefAsProp} from 'shared/ReactFeatureFlags';
import {enableRefAsProp, disableStringRefs} from 'shared/ReactFeatureFlags';

import {
createWorkInProgress,
Expand Down Expand Up @@ -266,6 +266,7 @@ function coerceRef(

let coercedRef;
if (
!disableStringRefs &&
mixedRef !== null &&
typeof mixedRef !== 'function' &&
typeof mixedRef !== 'object'
Expand Down
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
enableFloat,
enableLegacyHidden,
alwaysThrottleRetries,
disableStringRefs,
} from 'shared/ReactFeatureFlags';
import {
FunctionComponent,
Expand Down Expand Up @@ -1624,7 +1625,11 @@ function commitAttachRef(finishedWork: Fiber) {
}
} else {
if (__DEV__) {
if (!ref.hasOwnProperty('current')) {
// TODO: We should move these warnings to happen during the render
// phase (markRef).
if (disableStringRefs && typeof ref === 'string') {
console.error('String refs are no longer supported.');
} else if (!ref.hasOwnProperty('current')) {
console.error(
'Unexpected ref object provided for %s. ' +
'Use either a ref-setter function or React.createRef().',
Expand Down
22 changes: 22 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactFiberRefs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ describe('ReactFiberRefs', () => {
});

// @gate enableRefAsProp
// @gate !disableStringRefs
test('string ref props are converted to function refs', async () => {
let refProp;
function Child({ref}) {
Expand All @@ -112,4 +113,25 @@ describe('ReactFiberRefs', () => {
expect(typeof refProp === 'function').toBe(true);
expect(owner.refs.child.type).toBe('div');
});

// @gate disableStringRefs
test('log an error in dev if a string ref is passed to a ref-receiving component', async () => {
let refProp;
function Child({ref}) {
refProp = ref;
return <div ref={ref} />;
}

class Owner extends React.Component {
render() {
return <Child ref="child" />;
}
}

const root = ReactNoop.createRoot();
await expect(async () => {
await expect(act(() => root.render(<Owner />))).rejects.toThrow();
}).toErrorDev('String refs are no longer supported');
expect(refProp).toBe('child');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -1346,6 +1346,7 @@ describe('ReactIncrementalSideEffects', () => {
// TODO: Test that mounts, updates, refs, unmounts and deletions happen in the
// expected way for aborted and resumed render life-cycles.

// @gate !disableStringRefs
it('supports string refs', async () => {
let fooInstance = null;

Expand Down
37 changes: 19 additions & 18 deletions packages/react/src/__tests__/ReactCoffeeScriptClass-test.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -535,25 +535,26 @@ describe 'ReactCoffeeScriptClass', ->

test React.createElement(Foo), 'DIV', 'bar-through-context'

it 'supports string refs', ->
class Foo extends React.Component
render: ->
React.createElement(InnerComponent,
name: 'foo'
ref: 'inner'
)
if !featureFlags.disableStringRefs
it 'supports string refs', ->
class Foo extends React.Component
render: ->
React.createElement(InnerComponent,
name: 'foo'
ref: 'inner'
)

ref = React.createRef()
expect(->
test(React.createElement(Foo, ref: ref), 'DIV', 'foo')
).toErrorDev([
'Warning: Component "Foo" contains the string ref "inner". ' +
'Support for string refs will be removed in a future major release. ' +
'We recommend using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
' in Foo (at **)'
]);
expect(ref.current.refs.inner.getName()).toBe 'foo'
ref = React.createRef()
expect(->
test(React.createElement(Foo, ref: ref), 'DIV', 'foo')
).toErrorDev([
'Warning: Component "Foo" contains the string ref "inner". ' +
'Support for string refs will be removed in a future major release. ' +
'We recommend using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
' in Foo (at **)'
]);
expect(ref.current.refs.inner.getName()).toBe 'foo'

it 'supports drilling through to the DOM using findDOMNode', ->
ref = React.createRef()
Expand Down
36 changes: 19 additions & 17 deletions packages/react/src/__tests__/ReactES6Class-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -576,24 +576,26 @@ describe('ReactES6Class', () => {
});
}

it('supports string refs', () => {
class Foo extends React.Component {
render() {
return <Inner name="foo" ref="inner" />;
if (!require('shared/ReactFeatureFlags').disableStringRefs) {
it('supports string refs', () => {
class Foo extends React.Component {
render() {
return <Inner name="foo" ref="inner" />;
}
}
}
const ref = React.createRef();
expect(() => {
test(<Foo ref={ref} />, 'DIV', 'foo');
}).toErrorDev([
'Warning: Component "Foo" contains the string ref "inner". ' +
'Support for string refs will be removed in a future major release. ' +
'We recommend using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
' in Foo (at **)',
]);
expect(ref.current.refs.inner.getName()).toBe('foo');
});
const ref = React.createRef();
expect(() => {
test(<Foo ref={ref} />, 'DIV', 'foo');
}).toErrorDev([
'Warning: Component "Foo" contains the string ref "inner". ' +
'Support for string refs will be removed in a future major release. ' +
'We recommend using useRef() or createRef() instead. ' +
'Learn more about using refs safely here: https://reactjs.org/link/strict-mode-string-ref\n' +
' in Foo (at **)',
]);
expect(ref.current.refs.inner.getName()).toBe('foo');
});
}

it('supports drilling through to the DOM using findDOMNode', () => {
const ref = React.createRef();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,17 @@ describe('ReactProfiler DevTools integration', () => {

Scheduler.unstable_advanceTime(20);

function Throws() {
throw new Error('Oops!');
}

expect(() => {
rendered.update(
<div ref="this-will-cause-an-error">
<Throws>
<AdvanceTime byAmount={3} />
</div>,
</Throws>,
);
}).toThrow();
}).toThrow('Oops!');

Scheduler.unstable_advanceTime(20);

Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/__tests__/ReactStrictMode-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,7 @@ describe('string refs', () => {
act = require('internal-test-utils').act;
});

// @gate !disableStringRefs
it('should warn within a strict tree', async () => {
const {StrictMode} = React;

Expand Down Expand Up @@ -1006,6 +1007,7 @@ describe('string refs', () => {
});
});

// @gate !disableStringRefs
it('should warn within a strict tree', async () => {
const {StrictMode} = React;

Expand Down
Loading

0 comments on commit c979895

Please sign in to comment.