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.

Sign up for GitHub

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

Convert refs-destruction to createRoot #28011

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Convert refs-destruction to createRoot #28011

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
107 changes: 69 additions & 38 deletions packages/react-dom/src/__tests__/refs-destruction-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,22 @@

let React;
let ReactDOM;
let ReactDOMClient;
let ReactTestUtils;

let TestComponent;
let act;
let theInnerDivRef;
let theInnerClassComponentRef;

describe('refs-destruction', () => {
beforeEach(() => {
jest.resetModules();

React = require('react');
ReactDOM = require('react-dom');
ReactDOMClient = require('react-dom/client');
ReactTestUtils = require('react-dom/test-utils');
act = require('internal-test-utils').act;

class ClassComponent extends React.Component {
render() {
Expand All @@ -30,8 +35,11 @@ describe('refs-destruction', () => {
}

TestComponent = class extends React.Component {
theInnerDivRef = React.createRef();
theInnerClassComponentRef = React.createRef();
constructor(props) {
super(props);
theInnerDivRef = React.createRef();
theInnerClassComponentRef = React.createRef();
}

render() {
if (this.props.destroy) {
Expand All @@ -46,77 +54,96 @@ describe('refs-destruction', () => {
} else {
return (
<div>
<div ref={this.theInnerDivRef} />
<ClassComponent ref={this.theInnerClassComponentRef} />
<div ref={theInnerDivRef} />
<ClassComponent ref={theInnerClassComponentRef} />
</div>
);
}
}
};
});

it('should remove refs when destroying the parent', () => {
afterEach(() => {
theInnerClassComponentRef = null;
theInnerDivRef = null;
});

it('should remove refs when destroying the parent', async () => {
const container = document.createElement('div');
const testInstance = ReactDOM.render(<TestComponent />, container);
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<TestComponent />);
});

expect(
ReactTestUtils.isDOMComponent(testInstance.theInnerDivRef.current),
).toBe(true);
expect(testInstance.theInnerClassComponentRef.current).toBeTruthy();
expect(ReactTestUtils.isDOMComponent(theInnerDivRef.current)).toBe(true);
expect(theInnerClassComponentRef.current).toBeTruthy();

ReactDOM.unmountComponentAtNode(container);
root.unmount();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this works, I thought unmount is auto-batched like render.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, we wrap unmount in flushSync. If there was a render before unmount that hasn't flushed yet, then it would be batched with the sync flush for unmount:

react/packages/react-dom/src/client/ReactDOMRoot.js

Line 155 in f498aa2

flushSync(() => {


expect(testInstance.theInnerDivRef.current).toBe(null);
expect(testInstance.theInnerClassComponentRef.current).toBe(null);
expect(theInnerDivRef.current).toBe(null);
expect(theInnerClassComponentRef.current).toBe(null);
});

it('should remove refs when destroying the child', () => {
it('should remove refs when destroying the child', async () => {
const container = document.createElement('div');
const testInstance = ReactDOM.render(<TestComponent />, container);
expect(
ReactTestUtils.isDOMComponent(testInstance.theInnerDivRef.current),
).toBe(true);
expect(testInstance.theInnerClassComponentRef.current).toBeTruthy();
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<TestComponent />);
});

expect(ReactTestUtils.isDOMComponent(theInnerDivRef.current)).toBe(true);
expect(theInnerClassComponentRef.current).toBeTruthy();

ReactDOM.render(<TestComponent destroy={true} />, container);
await act(async () => {
root.render(<TestComponent destroy={true} />);
});

expect(testInstance.theInnerDivRef.current).toBe(null);
expect(testInstance.theInnerClassComponentRef.current).toBe(null);
expect(theInnerDivRef.current).toBe(null);
expect(theInnerClassComponentRef.current).toBe(null);
});

it('should remove refs when removing the child ref attribute', () => {
it('should remove refs when removing the child ref attribute', async () => {
const container = document.createElement('div');
const testInstance = ReactDOM.render(<TestComponent />, container);
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<TestComponent />);
});

expect(
ReactTestUtils.isDOMComponent(testInstance.theInnerDivRef.current),
).toBe(true);
expect(testInstance.theInnerClassComponentRef.current).toBeTruthy();
expect(ReactTestUtils.isDOMComponent(theInnerDivRef.current)).toBe(true);
expect(theInnerClassComponentRef.current).toBeTruthy();

ReactDOM.render(<TestComponent removeRef={true} />, container);
await act(async () => {
root.render(<TestComponent removeRef={true} />);
});

expect(testInstance.theInnerDivRef.current).toBe(null);
expect(testInstance.theInnerClassComponentRef.current).toBe(null);
expect(theInnerDivRef.current).toBe(null);
expect(theInnerClassComponentRef.current).toBe(null);
});

it('should not error when destroying child with ref asynchronously', () => {
it('should not error when destroying child with ref asynchronously', async () => {
let nestedRoot;
class Modal extends React.Component {
componentDidMount() {
this.div = document.createElement('div');
nestedRoot = ReactDOMClient.createRoot(this.div);
document.body.appendChild(this.div);
this.componentDidUpdate();
}

componentDidUpdate() {
ReactDOM.render(<div>{this.props.children}</div>, this.div);
setTimeout(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clowning af but the test before was clowny af (rendering and unmounting roots in lifecycles) so I don't want to rewrite it and risk losing whatever it was testing for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be flushSync because you can't use async/await in lifecycles. Needs to be a timeout because we warn if you use flushSync in lifecycles. The act flush ensures the timeout is fired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what's being tested here. We could also mark it as legacy and gate the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's testing that if you have a nested root that you animate on unmount, there's no error. This is how you did portals before createPortal, so I don't think it's legacy until we remove class components.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kassens but I'm curious what you mean by gating, if this was legacy what gate would we use?

ReactDOM.flushSync(() => {
nestedRoot.render(<div>{this.props.children}</div>);
});
}, 0);
}

componentWillUnmount() {
const self = this;
// some async animation
setTimeout(function () {
expect(function () {
ReactDOM.unmountComponentAtNode(self.div);
nestedRoot.unmount();
}).not.toThrow();
document.body.removeChild(self.div);
}, 0);
Expand Down Expand Up @@ -144,8 +171,12 @@ describe('refs-destruction', () => {
}

const container = document.createElement('div');
ReactDOM.render(<App />, container);
ReactDOM.render(<App hidden={true} />, container);
jest.runAllTimers();
const root = ReactDOMClient.createRoot(container);
await act(async () => {
root.render(<App />);
});
await act(async () => {
root.render(<App hidden={true} />);
});
});
});