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

[string-refs] cleanup string ref code #31443

Merged
merged 1 commit into from
Nov 6, 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
11 changes: 0 additions & 11 deletions packages/jest-react/src/JestReact.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/

import {REACT_ELEMENT_TYPE, REACT_FRAGMENT_TYPE} from 'shared/ReactSymbols';
import {disableStringRefs} from 'shared/ReactFeatureFlags';
const {assertConsoleLogsCleared} = require('internal-test-utils/consoleMock');

import isArray from 'shared/isArray';
Expand Down Expand Up @@ -56,23 +55,13 @@ function createJSXElementForTestComparison(type, props) {
value: null,
});
return element;
} else if (!__DEV__ && disableStringRefs) {
return {
$$typeof: REACT_ELEMENT_TYPE,
type: type,
key: null,
ref: null,
props: props,
};
} else {
return {
$$typeof: REACT_ELEMENT_TYPE,
type: type,
key: null,
ref: null,
props: props,
_owner: null,
_store: __DEV__ ? {} : undefined,
};
}
}
Expand Down
14 changes: 0 additions & 14 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ import type {Postpone} from 'react/src/ReactPostpone';
import type {TemporaryReferenceSet} from './ReactFlightTemporaryReferences';

import {
disableStringRefs,
enableBinaryFlight,
enablePostpone,
enableFlightReadableStream,
Expand Down Expand Up @@ -688,16 +687,6 @@ function createElement(
enumerable: false,
get: nullRefGetter,
});
} else if (!__DEV__ && disableStringRefs) {
element = ({
// This tag allows us to uniquely identify this as a React Element
$$typeof: REACT_ELEMENT_TYPE,

type,
key,
ref: null,
props,
}: any);
} else {
element = ({
// This tag allows us to uniquely identify this as a React Element
Expand All @@ -707,9 +696,6 @@ function createElement(
key,
ref: null,
props,

// Record the component responsible for creating this element.
_owner: __DEV__ && owner === null ? response._debugRootOwner : owner,
}: any);
}

Expand Down
4 changes: 1 addition & 3 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3268,9 +3268,7 @@ describe('ReactFlight', () => {
expect(greeting._owner).toBe(greeting._debugInfo[0]);
} else {
expect(greeting._debugInfo).toBe(undefined);
expect(greeting._owner).toBe(
gate(flags => flags.disableStringRefs) ? undefined : null,
);
expect(greeting._owner).toBe(undefined);
}
ReactNoop.render(greeting);
});
Expand Down
112 changes: 0 additions & 112 deletions packages/react-dom/src/__tests__/ReactComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,6 @@ describe('ReactComponent', () => {
}).toThrowError(/Target container is not a DOM element./);
});

// @gate !disableStringRefs
it('should throw when supplying a string ref outside of render method', async () => {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await expect(
act(() => {
root.render(<div ref="badDiv" />);
}),
// TODO: This throws an AggregateError. Need to update test infra to
// support matching against AggregateError.
).rejects.toThrow();
});

it('should throw (in dev) when children are mutated during render', async () => {
function Wrapper(props) {
props.children[1] = <p key={1} />; // Mutation is illegal
Expand Down Expand Up @@ -132,105 +119,6 @@ describe('ReactComponent', () => {
}
});

// @gate !disableStringRefs
it('string refs do not detach and reattach on every render', async () => {
let refVal;
class Child extends React.Component {
componentDidUpdate() {
// The parent ref should still be attached because it hasn't changed
// since the last render. If the ref had changed, then this would be
// undefined because refs are attached during the same phase (layout)
// as componentDidUpdate, in child -> parent order. So the new parent
// ref wouldn't have attached yet.
refVal = this.props.contextRef();
}

render() {
if (this.props.show) {
return <div>child</div>;
}
}
}

class Parent extends React.Component {
render() {
return (
<div id="test-root" ref="root">
<Child
contextRef={() => this.refs.root}
show={this.props.showChild}
/>
</div>
);
}
}

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

await act(() => {
root.render(<Parent />);
});

assertConsoleErrorDev(['contains the string ref']);

expect(refVal).toBe(undefined);
await act(() => {
root.render(<Parent showChild={true} />);
});
expect(refVal).toBe(container.querySelector('#test-root'));
});

// @gate !disableStringRefs
it('should support string refs on owned components', async () => {
const innerObj = {};
const outerObj = {};

class Wrapper extends React.Component {
getObject = () => {
return this.props.object;
};

render() {
return <div>{this.props.children}</div>;
}
}

class Component extends React.Component {
render() {
const inner = <Wrapper object={innerObj} ref="inner" />;
const outer = (
<Wrapper object={outerObj} ref="outer">
{inner}
</Wrapper>
);
return outer;
}

componentDidMount() {
expect(this.refs.inner.getObject()).toEqual(innerObj);
expect(this.refs.outer.getObject()).toEqual(outerObj);
}
}

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await expect(async () => {
await act(() => {
root.render(<Component />);
});
}).toErrorDev([
'Component "Component" 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://react.dev/link/strict-mode-string-ref\n' +
' in Wrapper (at **)\n' +
' in div (at **)\n' +
' in Wrapper (at **)\n' +
' in Component (at **)',
]);
});

it('should not have string refs on unmounted components', async () => {
class Parent extends React.Component {
render() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -537,11 +537,8 @@ describe('ReactCompositeComponent', () => {
});

it('should cleanup even if render() fatals', async () => {
const dispatcherEnabled =
__DEV__ ||
!gate(flags => flags.disableStringRefs) ||
gate(flags => flags.enableCache);
const ownerEnabled = __DEV__ || !gate(flags => flags.disableStringRefs);
const dispatcherEnabled = __DEV__ || gate(flags => flags.enableCache);
const ownerEnabled = __DEV__;

let stashedDispatcher;
class BadComponent extends React.Component {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,8 @@ function initModules() {
};
}

const {
resetModules,
asyncReactDOMRender,
clientRenderOnServerString,
expectMarkupMatch,
} = ReactDOMServerIntegrationUtils(initModules);
const {resetModules, clientRenderOnServerString, expectMarkupMatch} =
ReactDOMServerIntegrationUtils(initModules);

describe('ReactDOMServerIntegration', () => {
beforeEach(() => {
Expand Down Expand Up @@ -75,36 +71,6 @@ describe('ReactDOMServerIntegration', () => {
expect(refElement).not.toBe(null);
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() {
return <div ref="myDiv" />;
}
}

const markup = ReactDOMServer.renderToString(<RefsComponent />);
const root = document.createElement('div');
root.innerHTML = markup;
let component = null;
resetModules();
await expect(async () => {
await asyncReactDOMRender(
<RefsComponent ref={e => (component = e)} />,
root,
true,
);
}).toErrorDev([
'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://react.dev/link/strict-mode-string-ref\n' +
' in div (at **)\n' +
' in RefsComponent (at **)',
]);
expect(component.refs.myDiv).toBe(root.firstChild);
});
});

it('should forward refs', async () => {
Expand Down
Loading
Loading