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

Remove string refs (behind flag) #28322

Merged
merged 2 commits into from
Feb 27, 2024
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
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