From d9bddd027ace29b5e36a819bc14a3a5e11a27621 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Thu, 25 Jan 2024 15:55:51 -0500 Subject: [PATCH 1/3] Use createRoot in ReactEmptyComponent-test --- .../src/__tests__/ReactEmptyComponent-test.js | 201 +++++++++++------- 1 file changed, 126 insertions(+), 75 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js b/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js index 93bc493140501..223f5237c05e8 100644 --- a/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js @@ -11,10 +11,13 @@ let React; let ReactDOM; +let ReactDOMClient; let ReactTestUtils; let TogglingComponent; +let act; let log; +let container; describe('ReactEmptyComponent', () => { beforeEach(() => { @@ -22,10 +25,14 @@ describe('ReactEmptyComponent', () => { React = require('react'); ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactTestUtils = require('react-dom/test-utils'); + act = require('internal-test-utils').act; log = jest.fn(); + container = document.createElement('div'); + TogglingComponent = class extends React.Component { state = {component: this.props.firstComponent}; @@ -47,40 +54,44 @@ describe('ReactEmptyComponent', () => { describe.each([null, undefined])('when %s', nullORUndefined => { it('should not throw when rendering', () => { - class Component extends React.Component { - render() { - return nullORUndefined; - } + function EmptyComponent() { + return nullORUndefined; } - expect(function () { - ReactTestUtils.renderIntoDocument(); + const root = ReactDOMClient.createRoot(container); + + expect(() => { + ReactDOM.flushSync(() => { + root.render(); + }); }).not.toThrowError(); }); - it('should not produce child DOM nodes for nullish and false', () => { - class Component1 extends React.Component { - render() { - return nullORUndefined; - } + it('should not produce child DOM nodes for nullish and false', async () => { + function Component1() { + return nullORUndefined; } - class Component2 extends React.Component { - render() { - return false; - } + function Component2() { + return false; } const container1 = document.createElement('div'); - ReactDOM.render(, container1); + const root1 = ReactDOMClient.createRoot(container1); + await act(() => { + root1.render(); + }); expect(container1.children.length).toBe(0); const container2 = document.createElement('div'); - ReactDOM.render(, container2); + const root2 = ReactDOMClient.createRoot(container2); + await act(() => { + root2.render(); + }); expect(container2.children.length).toBe(0); }); - it('should be able to switch between rendering nullish and a normal tag', () => { + it('should be able to switch between rendering nullish and a normal tag', async () => { const instance1 = ( { /> ); - ReactTestUtils.renderIntoDocument(instance1); - ReactTestUtils.renderIntoDocument(instance2); + const container2 = document.createElement('div'); + const root1 = ReactDOMClient.createRoot(container); + await act(() => { + root1.render(instance1); + }); + + const root2 = ReactDOMClient.createRoot(container2); + await act(() => { + root2.render(instance2); + }); expect(log).toHaveBeenCalledTimes(4); expect(log).toHaveBeenNthCalledWith(1, null); @@ -110,7 +129,7 @@ describe('ReactEmptyComponent', () => { expect(log).toHaveBeenNthCalledWith(4, null); }); - it('should be able to switch in a list of children', () => { + it('should be able to switch in a list of children', async () => { const instance1 = ( { /> ); - ReactTestUtils.renderIntoDocument( -
- {instance1} - {instance1} - {instance1} -
, - ); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( +
+ {instance1} + {instance1} + {instance1} +
, + ); + }); expect(log).toHaveBeenCalledTimes(6); expect(log).toHaveBeenNthCalledWith(1, null); @@ -158,11 +180,19 @@ describe('ReactEmptyComponent', () => { /> ); - expect(function () { - ReactTestUtils.renderIntoDocument(instance1); + const root1 = ReactDOMClient.createRoot(container); + expect(() => { + ReactDOM.flushSync(() => { + root1.render(instance1); + }); }).not.toThrow(); - expect(function () { - ReactTestUtils.renderIntoDocument(instance2); + + const container2 = document.createElement('div'); + const root2 = ReactDOMClient.createRoot(container2); + expect(() => { + ReactDOM.flushSync(() => { + root2.render(instance2); + }); }).not.toThrow(); expect(log).toHaveBeenCalledTimes(4); @@ -182,16 +212,12 @@ describe('ReactEmptyComponent', () => { 'should have findDOMNode return null when multiple layers of composite ' + 'components render to the same nullish placeholder', () => { - class GrandChild extends React.Component { - render() { - return nullORUndefined; - } + function GrandChild() { + return nullORUndefined; } - class Child extends React.Component { - render() { - return ; - } + function Child() { + return ; } const instance1 = ( @@ -201,11 +227,19 @@ describe('ReactEmptyComponent', () => { ); - expect(function () { - ReactTestUtils.renderIntoDocument(instance1); + const root1 = ReactDOMClient.createRoot(container); + expect(() => { + ReactDOM.flushSync(() => { + root1.render(instance1); + }); }).not.toThrow(); - expect(function () { - ReactTestUtils.renderIntoDocument(instance2); + + const container2 = document.createElement('div'); + const root2 = ReactDOMClient.createRoot(container2); + expect(() => { + ReactDOM.flushSync(() => { + root2.render(instance2); + }); }).not.toThrow(); expect(log).toHaveBeenCalledTimes(4); @@ -222,8 +256,8 @@ describe('ReactEmptyComponent', () => { }, ); - it('works when switching components', () => { - let assertions = 0; + it('works when switching components', async () => { + let innerRef; class Inner extends React.Component { render() { @@ -234,44 +268,51 @@ describe('ReactEmptyComponent', () => { // Make sure the DOM node resolves properly even if we're replacing a // `null` component expect(ReactDOM.findDOMNode(this)).not.toBe(null); - assertions++; } componentWillUnmount() { // Even though we're getting replaced by `null`, we haven't been // replaced yet! expect(ReactDOM.findDOMNode(this)).not.toBe(null); - assertions++; } } - class Wrapper extends React.Component { - render() { - return this.props.showInner ? : nullORUndefined; - } + function Wrapper({showInner}) { + innerRef = React.createRef(null); + return showInner ? : nullORUndefined; } const el = document.createElement('div'); - let component; // Render the component... - component = ReactDOM.render(, el); - expect(ReactDOM.findDOMNode(component)).not.toBe(null); + const root = ReactDOMClient.createRoot(el); + await act(() => { + root.render(); + }); + expect(innerRef.current).not.toBe(null); // Switch to null... - component = ReactDOM.render(, el); - expect(ReactDOM.findDOMNode(component)).toBe(null); + await act(() => { + root.render(); + }); + expect(innerRef.current).toBe(null); // ...then switch back. - component = ReactDOM.render(, el); - expect(ReactDOM.findDOMNode(component)).not.toBe(null); + await act(() => { + root.render(); + }); + expect(innerRef.current).not.toBe(null); - expect(assertions).toBe(3); + expect.assertions(6); }); - it('can render nullish at the top level', () => { + it('can render nullish at the top level', async () => { const div = document.createElement('div'); - ReactDOM.render(nullORUndefined, div); + const root = ReactDOMClient.createRoot(div); + + await act(() => { + root.render(nullORUndefined); + }); expect(div.innerHTML).toBe(''); }); @@ -308,26 +349,30 @@ describe('ReactEmptyComponent', () => { } } - expect(function () { - ReactTestUtils.renderIntoDocument(); + const root = ReactDOMClient.createRoot(container); + expect(() => { + ReactDOM.flushSync(() => { + root.render(); + }); }).not.toThrow(); }); - it('preserves the dom node during updates', () => { - class Empty extends React.Component { - render() { - return nullORUndefined; - } + it('preserves the dom node during updates', async () => { + function Empty() { + return nullORUndefined; } - const container = document.createElement('div'); - - ReactDOM.render(, container); + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); const noscript1 = container.firstChild; expect(noscript1).toBe(null); // This update shouldn't create a DOM node - ReactDOM.render(, container); + await act(() => { + root.render(); + }); const noscript2 = container.firstChild; expect(noscript2).toBe(null); }); @@ -338,8 +383,11 @@ describe('ReactEmptyComponent', () => { }; const EmptyForwardRef = React.forwardRef(Empty); + const root = ReactDOMClient.createRoot(container); expect(() => { - ReactTestUtils.renderIntoDocument(); + ReactDOM.flushSync(() => { + root.render(); + }); }).not.toThrowError(); }); @@ -349,8 +397,11 @@ describe('ReactEmptyComponent', () => { }; const EmptyMemo = React.memo(Empty); + const root = ReactDOMClient.createRoot(container); expect(() => { - ReactTestUtils.renderIntoDocument(); + ReactDOM.flushSync(() => { + root.render(); + }); }).not.toThrowError(); }); }); From 9dd38b6cd5f332b48781ff64d277adf99424bee4 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Thu, 25 Jan 2024 16:34:08 -0500 Subject: [PATCH 2/3] Use scheduler log --- .../src/__tests__/ReactEmptyComponent-test.js | 88 +++++++------------ 1 file changed, 34 insertions(+), 54 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js b/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js index 223f5237c05e8..8903db914f615 100644 --- a/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js @@ -15,6 +15,8 @@ let ReactDOMClient; let ReactTestUtils; let TogglingComponent; let act; +let Scheduler; +let assertLog; let log; let container; @@ -27,9 +29,10 @@ describe('ReactEmptyComponent', () => { ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); ReactTestUtils = require('react-dom/test-utils'); - act = require('internal-test-utils').act; - - log = jest.fn(); + Scheduler = require('scheduler'); + const InternalTestUtils = require('internal-test-utils'); + act = InternalTestUtils.act; + assertLog = InternalTestUtils.assertLog; container = document.createElement('div'); @@ -37,12 +40,12 @@ describe('ReactEmptyComponent', () => { state = {component: this.props.firstComponent}; componentDidMount() { - log(ReactDOM.findDOMNode(this)); + Scheduler.log('mount ' + ReactDOM.findDOMNode(this)?.nodeName); this.setState({component: this.props.secondComponent}); } componentDidUpdate() { - log(ReactDOM.findDOMNode(this)); + Scheduler.log('update ' + ReactDOM.findDOMNode(this)?.nodeName); } render() { @@ -116,17 +119,12 @@ describe('ReactEmptyComponent', () => { root2.render(instance2); }); - expect(log).toHaveBeenCalledTimes(4); - expect(log).toHaveBeenNthCalledWith(1, null); - expect(log).toHaveBeenNthCalledWith( - 2, - expect.objectContaining({tagName: 'DIV'}), - ); - expect(log).toHaveBeenNthCalledWith( - 3, - expect.objectContaining({tagName: 'DIV'}), - ); - expect(log).toHaveBeenNthCalledWith(4, null); + assertLog([ + 'mount undefined', + 'update DIV', + 'mount DIV', + 'update undefined', + ]); }); it('should be able to switch in a list of children', async () => { @@ -148,22 +146,14 @@ describe('ReactEmptyComponent', () => { ); }); - expect(log).toHaveBeenCalledTimes(6); - expect(log).toHaveBeenNthCalledWith(1, null); - expect(log).toHaveBeenNthCalledWith(2, null); - expect(log).toHaveBeenNthCalledWith(3, null); - expect(log).toHaveBeenNthCalledWith( - 4, - expect.objectContaining({tagName: 'DIV'}), - ); - expect(log).toHaveBeenNthCalledWith( - 5, - expect.objectContaining({tagName: 'DIV'}), - ); - expect(log).toHaveBeenNthCalledWith( - 6, - expect.objectContaining({tagName: 'DIV'}), - ); + assertLog([ + 'mount undefined', + 'mount undefined', + 'mount undefined', + 'update DIV', + 'update DIV', + 'update DIV', + ]); }); it('should distinguish between a script placeholder and an actual script tag', () => { @@ -195,17 +185,12 @@ describe('ReactEmptyComponent', () => { }); }).not.toThrow(); - expect(log).toHaveBeenCalledTimes(4); - expect(log).toHaveBeenNthCalledWith(1, null); - expect(log).toHaveBeenNthCalledWith( - 2, - expect.objectContaining({tagName: 'SCRIPT'}), - ); - expect(log).toHaveBeenNthCalledWith( - 3, - expect.objectContaining({tagName: 'SCRIPT'}), - ); - expect(log).toHaveBeenNthCalledWith(4, null); + assertLog([ + 'mount undefined', + 'update SCRIPT', + 'mount SCRIPT', + 'update undefined', + ]); }); it( @@ -242,17 +227,12 @@ describe('ReactEmptyComponent', () => { }); }).not.toThrow(); - expect(log).toHaveBeenCalledTimes(4); - expect(log).toHaveBeenNthCalledWith( - 1, - expect.objectContaining({tagName: 'DIV'}), - ); - expect(log).toHaveBeenNthCalledWith(2, null); - expect(log).toHaveBeenNthCalledWith(3, null); - expect(log).toHaveBeenNthCalledWith( - 4, - expect.objectContaining({tagName: 'DIV'}), - ); + assertLog([ + 'mount DIV', + 'update undefined', + 'mount undefined', + 'update DIV', + ]); }, ); From f337182437d39a004f915eb0a3e7e35bdcbb969b Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Thu, 25 Jan 2024 16:36:28 -0500 Subject: [PATCH 3/3] lint --- packages/react-dom/src/__tests__/ReactEmptyComponent-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js b/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js index 8903db914f615..9316ad4392f61 100644 --- a/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactEmptyComponent-test.js @@ -12,13 +12,11 @@ let React; let ReactDOM; let ReactDOMClient; -let ReactTestUtils; let TogglingComponent; let act; let Scheduler; let assertLog; -let log; let container; describe('ReactEmptyComponent', () => { @@ -28,7 +26,6 @@ describe('ReactEmptyComponent', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); - ReactTestUtils = require('react-dom/test-utils'); Scheduler = require('scheduler'); const InternalTestUtils = require('internal-test-utils'); act = InternalTestUtils.act;