From d8fbb952718c87fae952d06710debffb7cef8f6b Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Wed, 24 Jan 2024 12:37:46 -0500 Subject: [PATCH 1/4] Convert ReactMount to createRoot --- .../__tests__/ReactDOMHydrationDiff-test.js | 26 ++ .../src/__tests__/ReactDOMRoot-test.js | 233 +++++++----------- ...Mount-test.js => ReactLegacyMount-test.js} | 132 ++++++++++ 3 files changed, 251 insertions(+), 140 deletions(-) rename packages/react-dom/src/__tests__/{ReactMount-test.js => ReactLegacyMount-test.js} (69%) diff --git a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js index 45de6fa9104f4..d4bc242e96711 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js @@ -114,6 +114,32 @@ describe('ReactDOMServerHydration', () => { }); // @gate __DEV__ + it('warns when escaping on a checksum mismatch', () => { + function Mismatch({isClient}) { + if (isClient) { + return ( +
This markup contains an nbsp entity:   client text
+ ); + } + return ( +
This markup contains an nbsp entity:   server text
+ ); + } + + /* eslint-disable no-irregular-whitespace */ + expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` + [ + "Warning: Text content did not match. Server: "This markup contains an nbsp entity:   server text" Client: "This markup contains an nbsp entity:   client text" + in div (at **) + in Mismatch (at **)", + "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", + "Caught [Text content does not match server-rendered HTML.]", + "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", + ] + `); + /* eslint-enable no-irregular-whitespace */ + }); + it('warns when client and server render different html', () => { function Mismatch({isClient}) { return ( diff --git a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js index f00bb37af1ac0..7967cf84a2672 100644 --- a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js @@ -175,27 +175,6 @@ describe('ReactDOMRoot', () => { ); }); - it('clears existing children with legacy API', async () => { - container.innerHTML = '
a
b
'; - ReactDOM.render( -
- c - d -
, - container, - ); - expect(container.textContent).toEqual('cd'); - ReactDOM.render( -
- d - c -
, - container, - ); - await waitForAll([]); - expect(container.textContent).toEqual('dc'); - }); - it('clears existing children', async () => { container.innerHTML = '
a
b
'; const root = ReactDOMClient.createRoot(container); @@ -223,122 +202,6 @@ describe('ReactDOMRoot', () => { }).toThrow('createRoot(...): Target container is not a DOM element.'); }); - it('warns when rendering with legacy API into createRoot() container', async () => { - const root = ReactDOMClient.createRoot(container); - root.render(
Hi
); - await waitForAll([]); - expect(container.textContent).toEqual('Hi'); - expect(() => { - ReactDOM.render(
Bye
, container); - }).toErrorDev( - [ - // We care about this warning: - 'You are calling ReactDOM.render() on a container that was previously ' + - 'passed to ReactDOMClient.createRoot(). This is not supported. ' + - 'Did you mean to call root.render(element)?', - // This is more of a symptom but restructuring the code to avoid it isn't worth it: - 'Replacing React-rendered children with a new root component.', - ], - {withoutStack: true}, - ); - await waitForAll([]); - // This works now but we could disallow it: - expect(container.textContent).toEqual('Bye'); - }); - - it('warns when hydrating with legacy API into createRoot() container', async () => { - const root = ReactDOMClient.createRoot(container); - root.render(
Hi
); - await waitForAll([]); - expect(container.textContent).toEqual('Hi'); - expect(() => { - ReactDOM.hydrate(
Hi
, container); - }).toErrorDev( - [ - // We care about this warning: - 'You are calling ReactDOM.hydrate() on a container that was previously ' + - 'passed to ReactDOMClient.createRoot(). This is not supported. ' + - 'Did you mean to call hydrateRoot(container, element)?', - // This is more of a symptom but restructuring the code to avoid it isn't worth it: - 'Replacing React-rendered children with a new root component.', - ], - {withoutStack: true}, - ); - }); - - it('callback passed to legacy hydrate() API', () => { - container.innerHTML = '
Hi
'; - ReactDOM.hydrate(
Hi
, container, () => { - Scheduler.log('callback'); - }); - expect(container.textContent).toEqual('Hi'); - assertLog(['callback']); - }); - - it('warns when unmounting with legacy API (no previous content)', async () => { - const root = ReactDOMClient.createRoot(container); - root.render(
Hi
); - await waitForAll([]); - expect(container.textContent).toEqual('Hi'); - let unmounted = false; - expect(() => { - unmounted = ReactDOM.unmountComponentAtNode(container); - }).toErrorDev( - [ - // We care about this warning: - 'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' + - 'passed to ReactDOMClient.createRoot(). This is not supported. Did you mean to call root.unmount()?', - // This is more of a symptom but restructuring the code to avoid it isn't worth it: - "The node you're attempting to unmount was rendered by React and is not a top-level container.", - ], - {withoutStack: true}, - ); - expect(unmounted).toBe(false); - await waitForAll([]); - expect(container.textContent).toEqual('Hi'); - root.unmount(); - await waitForAll([]); - expect(container.textContent).toEqual(''); - }); - - it('warns when unmounting with legacy API (has previous content)', async () => { - // Currently createRoot().render() doesn't clear this. - container.appendChild(document.createElement('div')); - // The rest is the same as test above. - const root = ReactDOMClient.createRoot(container); - root.render(
Hi
); - await waitForAll([]); - expect(container.textContent).toEqual('Hi'); - let unmounted = false; - expect(() => { - unmounted = ReactDOM.unmountComponentAtNode(container); - }).toErrorDev( - [ - 'Did you mean to call root.unmount()?', - // This is more of a symptom but restructuring the code to avoid it isn't worth it: - "The node you're attempting to unmount was rendered by React and is not a top-level container.", - ], - {withoutStack: true}, - ); - expect(unmounted).toBe(false); - await waitForAll([]); - expect(container.textContent).toEqual('Hi'); - root.unmount(); - await waitForAll([]); - expect(container.textContent).toEqual(''); - }); - - it('warns when passing legacy container to createRoot()', () => { - ReactDOM.render(
Hi
, container); - expect(() => { - ReactDOMClient.createRoot(container); - }).toErrorDev( - 'You are calling ReactDOMClient.createRoot() on a container that was previously ' + - 'passed to ReactDOM.render(). This is not supported.', - {withoutStack: true}, - ); - }); - it('warns when creating two roots managing the same container', () => { ReactDOMClient.createRoot(container); expect(() => { @@ -399,6 +262,80 @@ describe('ReactDOMRoot', () => { } }); + it('should render different components in same root', async () => { + document.body.appendChild(container); + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render(
); + }); + expect(container.firstChild.nodeName).toBe('DIV'); + + await act(() => { + root.render(); + }); + expect(container.firstChild.nodeName).toBe('SPAN'); + }); + + it('should not warn if mounting into non-empty node', async () => { + container.innerHTML = '
'; + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(
); + }); + + expect(true).toBe(true); + }); + + it('should reuse markup if rendering to the same target twice', async () => { + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(
); + }); + const firstElm = container.firstChild; + await act(() => { + root.render(
); + }); + + expect(firstElm).toBe(container.firstChild); + }); + + it('should unmount and remount if the key changes', async () => { + function Component({text}) { + useEffect(() => { + Scheduler.log('Mount'); + + return () => { + Scheduler.log('Unmount'); + }; + }, []); + + return {text}; + } + + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render(); + }); + expect(container.firstChild.innerHTML).toBe('orange'); + assertLog(['Mount']); + + // If we change the key, the component is unmounted and remounted + await act(() => { + root.render(); + }); + expect(container.firstChild.innerHTML).toBe('green'); + assertLog(['Unmount', 'Mount']); + + // But if we don't change the key, the component instance is reused + await act(() => { + root.render(); + }); + expect(container.firstChild.innerHTML).toBe('blue'); + assertLog([]); + }); + it('throws if unmounting a root that has had its contents removed', async () => { const root = ReactDOMClient.createRoot(container); await act(() => { @@ -514,9 +451,6 @@ describe('ReactDOMRoot', () => { expect(() => ReactDOMClient.hydrateRoot(commentNode)).toThrow( 'hydrateRoot(...): Target container is not a DOM element.', ); - - // Still works in the legacy API - ReactDOM.render(
, commentNode); }); it('warn if no children passed to hydrateRoot', async () => { @@ -539,4 +473,23 @@ describe('ReactDOMRoot', () => { }, ); }); + + it('warns when given a function', () => { + function Component() { + return
; + } + + const root = ReactDOMClient.createRoot(document.createElement('div')); + + expect(() => { + ReactDOM.flushSync(() => { + root.render(Component); + }); + }).toErrorDev( + 'Functions are not valid as a React child. ' + + 'This may happen if you return a Component instead of from render. ' + + 'Or maybe you meant to call this function rather than return it.', + {withoutStack: true}, + ); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactMount-test.js b/packages/react-dom/src/__tests__/ReactLegacyMount-test.js similarity index 69% rename from packages/react-dom/src/__tests__/ReactMount-test.js rename to packages/react-dom/src/__tests__/ReactLegacyMount-test.js index b679466b25a30..20d777417172d 100644 --- a/packages/react-dom/src/__tests__/ReactMount-test.js +++ b/packages/react-dom/src/__tests__/ReactLegacyMount-test.js @@ -15,6 +15,10 @@ let React; let ReactDOM; let ReactDOMServer; let ReactTestUtils; +let Scheduler = require('scheduler'); +const ReactDOMClient = require('react-dom/client'); +let assertLog; +let waitForAll; describe('ReactMount', () => { beforeEach(() => { @@ -24,6 +28,11 @@ describe('ReactMount', () => { ReactDOM = require('react-dom'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); + Scheduler = require('scheduler'); + + const InternalTestUtils = require('internal-test-utils'); + assertLog = InternalTestUtils.assertLog; + waitForAll = InternalTestUtils.waitForAll; }); describe('unmountComponentAtNode', () => { @@ -337,4 +346,127 @@ describe('ReactMount', () => { ); }); }); + + it('clears existing children with legacy API', async () => { + const container = document.createElement('div'); + container.innerHTML = '
a
b
'; + ReactDOM.render( +
+ c + d +
, + container, + ); + expect(container.textContent).toEqual('cd'); + ReactDOM.render( +
+ d + c +
, + container, + ); + await waitForAll([]); + expect(container.textContent).toEqual('dc'); + }); + + it('warns when rendering with legacy API into createRoot() container', async () => { + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + root.render(
Hi
); + await waitForAll([]); + expect(container.textContent).toEqual('Hi'); + expect(() => { + ReactDOM.render(
Bye
, container); + }).toErrorDev( + [ + // We care about this warning: + 'You are calling ReactDOM.render() on a container that was previously ' + + 'passed to ReactDOMClient.createRoot(). This is not supported. ' + + 'Did you mean to call root.render(element)?', + // This is more of a symptom but restructuring the code to avoid it isn't worth it: + 'Replacing React-rendered children with a new root component.', + ], + {withoutStack: true}, + ); + await waitForAll([]); + // This works now but we could disallow it: + expect(container.textContent).toEqual('Bye'); + }); + + it('callback passed to legacy hydrate() API', () => { + const container = document.createElement('div'); + container.innerHTML = '
Hi
'; + ReactDOM.hydrate(
Hi
, container, () => { + Scheduler.log('callback'); + }); + expect(container.textContent).toEqual('Hi'); + assertLog(['callback']); + }); + + it('warns when unmounting with legacy API (no previous content)', async () => { + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + root.render(
Hi
); + await waitForAll([]); + expect(container.textContent).toEqual('Hi'); + let unmounted = false; + expect(() => { + unmounted = ReactDOM.unmountComponentAtNode(container); + }).toErrorDev( + [ + // We care about this warning: + 'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' + + 'passed to ReactDOMClient.createRoot(). This is not supported. Did you mean to call root.unmount()?', + // This is more of a symptom but restructuring the code to avoid it isn't worth it: + "The node you're attempting to unmount was rendered by React and is not a top-level container.", + ], + {withoutStack: true}, + ); + expect(unmounted).toBe(false); + await waitForAll([]); + expect(container.textContent).toEqual('Hi'); + root.unmount(); + await waitForAll([]); + expect(container.textContent).toEqual(''); + }); + + it('warns when unmounting with legacy API (has previous content)', async () => { + const container = document.createElement('div'); + // Currently createRoot().render() doesn't clear this. + container.appendChild(document.createElement('div')); + // The rest is the same as test above. + const root = ReactDOMClient.createRoot(container); + root.render(
Hi
); + await waitForAll([]); + expect(container.textContent).toEqual('Hi'); + let unmounted = false; + expect(() => { + unmounted = ReactDOM.unmountComponentAtNode(container); + }).toErrorDev( + [ + 'Did you mean to call root.unmount()?', + // This is more of a symptom but restructuring the code to avoid it isn't worth it: + "The node you're attempting to unmount was rendered by React and is not a top-level container.", + ], + {withoutStack: true}, + ); + expect(unmounted).toBe(false); + await waitForAll([]); + expect(container.textContent).toEqual('Hi'); + root.unmount(); + await waitForAll([]); + expect(container.textContent).toEqual(''); + }); + + it('warns when passing legacy container to createRoot()', () => { + const container = document.createElement('div'); + ReactDOM.render(
Hi
, container); + expect(() => { + ReactDOMClient.createRoot(container); + }).toErrorDev( + 'You are calling ReactDOMClient.createRoot() on a container that was previously ' + + 'passed to ReactDOM.render(). This is not supported.', + {withoutStack: true}, + ); + }); }); From 764effb0e1167b146a22842c8e26d936247afa7a Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Wed, 24 Jan 2024 12:37:46 -0500 Subject: [PATCH 2/4] Convert ReactMount to createRoot --- packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js index d4bc242e96711..0fde177f58b48 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js @@ -140,6 +140,7 @@ describe('ReactDOMServerHydration', () => { /* eslint-enable no-irregular-whitespace */ }); + // @gate __DEV__ it('warns when client and server render different html', () => { function Mismatch({isClient}) { return ( From 5119374ab2404ac071a8f74c7dcef7094091731b Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Wed, 24 Jan 2024 14:29:18 -0500 Subject: [PATCH 3/4] Fix tests --- packages/react-dom/src/__tests__/ReactLegacyMount-test.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactLegacyMount-test.js b/packages/react-dom/src/__tests__/ReactLegacyMount-test.js index 20d777417172d..78492811428c3 100644 --- a/packages/react-dom/src/__tests__/ReactLegacyMount-test.js +++ b/packages/react-dom/src/__tests__/ReactLegacyMount-test.js @@ -15,8 +15,8 @@ let React; let ReactDOM; let ReactDOMServer; let ReactTestUtils; -let Scheduler = require('scheduler'); -const ReactDOMClient = require('react-dom/client'); +let Scheduler; +let ReactDOMClient; let assertLog; let waitForAll; @@ -26,6 +26,7 @@ describe('ReactMount', () => { React = require('react'); ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); Scheduler = require('scheduler'); From 436c433b95eb46e813ca95f7bffe0001344bb43f Mon Sep 17 00:00:00 2001 From: Ricky Hanlon Date: Wed, 24 Jan 2024 14:40:27 -0500 Subject: [PATCH 4/4] No fallback to client render in www --- .../__tests__/ReactDOMHydrationDiff-test.js | 30 ++++++++++++------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js index 0fde177f58b48..0b6d5dabffe42 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js @@ -127,16 +127,26 @@ describe('ReactDOMServerHydration', () => { } /* eslint-disable no-irregular-whitespace */ - expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` - [ - "Warning: Text content did not match. Server: "This markup contains an nbsp entity:   server text" Client: "This markup contains an nbsp entity:   client text" - in div (at **) - in Mismatch (at **)", - "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", - "Caught [Text content does not match server-rendered HTML.]", - "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", - ] - `); + if (gate(flags => flags.enableClientRenderFallbackOnTextMismatch)) { + expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` + [ + "Warning: Text content did not match. Server: "This markup contains an nbsp entity:   server text" Client: "This markup contains an nbsp entity:   client text" + in div (at **) + in Mismatch (at **)", + "Warning: An error occurred during hydration. The server HTML was replaced with client content in
.", + "Caught [Text content does not match server-rendered HTML.]", + "Caught [There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.]", + ] + `); + } else { + expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` + [ + "Warning: Text content did not match. Server: "This markup contains an nbsp entity:   server text" Client: "This markup contains an nbsp entity:   client text" + in div (at **) + in Mismatch (at **)", + ] + `); + } /* eslint-enable no-irregular-whitespace */ });