From 098d967825929ca22002903f003b60443021ba25 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 24 Oct 2019 20:46:48 +0200 Subject: [PATCH 1/5] feat: Add option to render concurrent roots --- package.json | 4 ++-- src/__tests__/concurrent.js | 26 ++++++++++++++++++++++ src/pure.js | 43 +++++++++++++++++++++++++++++++------ tests/setup-env.js | 1 + 4 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 src/__tests__/concurrent.js diff --git a/package.json b/package.json index 53a43cb5..43ab162c 100644 --- a/package.json +++ b/package.json @@ -55,8 +55,8 @@ "cross-env": "^6.0.0", "kcd-scripts": "^1.7.0", "npm-run-all": "^4.1.5", - "react": "^16.9.0", - "react-dom": "^16.9.0", + "react": "^0.0.0-experimental-f6b8d31a7", + "react-dom": "^0.0.0-experimental-f6b8d31a7", "rimraf": "^3.0.0" }, "peerDependencies": { diff --git a/src/__tests__/concurrent.js b/src/__tests__/concurrent.js new file mode 100644 index 00000000..98c48f7b --- /dev/null +++ b/src/__tests__/concurrent.js @@ -0,0 +1,26 @@ +import React from 'react' +import {render} from '../' + +test('simple render works like legacy', () => { + const {container} = render(
test
, {root: 'concurrent'}) + + expect(container).toHaveTextContent('test') +}) + +test('unmount calls back after commit', () => { + const {container, unmount} = render(
test
, {root: 'concurrent'}) + + unmount(() => { + expect(container.children).toHaveLength(0) + }) + expect(container.children).toHaveLength(1) +}) + +test('rerender calls back after commit', () => { + const {container, rerender} = render(
foo
, {root: 'concurrent'}) + + rerender(
bar
, () => { + expect(container).toHaveTextContent('bar') + }) + expect(container).toHaveTextContent('foo') +}) diff --git a/src/pure.js b/src/pure.js index 1b1838bf..89488e60 100644 --- a/src/pure.js +++ b/src/pure.js @@ -19,6 +19,10 @@ configureDTL({ }) const mountedContainers = new Set() +/** + * @type {WeakMap} + */ +const mountedRoots = new WeakMap() function render( ui, @@ -28,6 +32,7 @@ function render( queries, hydrate = false, wrapper: WrapperComponent, + root, } = {}, ) { if (!baseElement) { @@ -44,13 +49,21 @@ function render( // they're passing us a custom container or not. mountedContainers.add(container) + let reactRoot = null + if (root === 'concurrent') { + reactRoot = ReactDOM.createRoot(container, {hydrate}) + mountedRoots.set(container, reactRoot) + } + const wrapUiIfNeeded = innerElement => WrapperComponent ? React.createElement(WrapperComponent, null, innerElement) : innerElement act(() => { - if (hydrate) { + if (reactRoot) { + reactRoot.render(wrapUiIfNeeded(ui)) + } else if (hydrate) { ReactDOM.hydrate(wrapUiIfNeeded(ui), container) } else { ReactDOM.render(wrapUiIfNeeded(ui), container) @@ -66,11 +79,21 @@ function render( el.forEach(e => console.log(prettyDOM(e))) : // eslint-disable-next-line no-console, console.log(prettyDOM(el)), - unmount: () => ReactDOM.unmountComponentAtNode(container), - rerender: rerenderUi => { - render(wrapUiIfNeeded(rerenderUi), {container, baseElement}) - // Intentionally do not return anything to avoid unnecessarily complicating the API. - // folks can use all the same utilities we return in the first place that are bound to the container + unmount: callback => { + if (reactRoot) { + reactRoot.unmount(callback) + } else { + ReactDOM.unmountComponentAtNode(container) + } + }, + rerender: (rerenderUi, callback) => { + if (reactRoot === null) { + render(wrapUiIfNeeded(rerenderUi), {container, baseElement}) + // Intentionally do not return anything to avoid unnecessarily complicating the API. + // folks can use all the same utilities we return in the first place that are bound to the container + } else { + reactRoot.render(wrapUiIfNeeded(rerenderUi), callback) + } }, asFragment: () => { /* istanbul ignore if (jsdom limitation) */ @@ -95,7 +118,13 @@ function cleanup() { // maybe one day we'll expose this (perhaps even as a utility returned by render). // but let's wait until someone asks for it. function cleanupAtContainer(container) { - ReactDOM.unmountComponentAtNode(container) + const reactRoot = mountedRoots.get(container) + if (reactRoot === undefined) { + ReactDOM.unmountComponentAtNode(container) + } else { + reactRoot.unmount() + } + if (container.parentNode === document.body) { document.body.removeChild(container) } diff --git a/tests/setup-env.js b/tests/setup-env.js index 264828a9..cc0b570c 100644 --- a/tests/setup-env.js +++ b/tests/setup-env.js @@ -1 +1,2 @@ import '@testing-library/jest-dom/extend-expect' +jest.mock('scheduler', () => require('scheduler/unstable_mock')) From 28706ffbe3465997bddbe26ae1f3d477c9143c53 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 24 Oct 2019 21:00:39 +0200 Subject: [PATCH 2/5] Disable this for now to make build green --- tests/setup-env.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/setup-env.js b/tests/setup-env.js index cc0b570c..442d82bd 100644 --- a/tests/setup-env.js +++ b/tests/setup-env.js @@ -1,2 +1,3 @@ import '@testing-library/jest-dom/extend-expect' +// eslint-disable-next-line import/no-extraneous-dependencies jest.mock('scheduler', () => require('scheduler/unstable_mock')) From be4b6c5e029b9f8345b01dd0e4815084c810cd29 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 25 Oct 2019 17:31:54 +0200 Subject: [PATCH 3/5] unmount needs to wrapped in act, rerender not --- src/__tests__/concurrent.js | 16 +++++++--------- src/pure.js | 10 ++++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/__tests__/concurrent.js b/src/__tests__/concurrent.js index 98c48f7b..41e9be83 100644 --- a/src/__tests__/concurrent.js +++ b/src/__tests__/concurrent.js @@ -7,20 +7,18 @@ test('simple render works like legacy', () => { expect(container).toHaveTextContent('test') }) -test('unmount calls back after commit', () => { +test('unmounts are flushed in sync', () => { const {container, unmount} = render(
test
, {root: 'concurrent'}) - unmount(() => { - expect(container.children).toHaveLength(0) - }) - expect(container.children).toHaveLength(1) + unmount() + + expect(container.children).toHaveLength(0) }) -test('rerender calls back after commit', () => { +test('rerender are flushed in sync', () => { const {container, rerender} = render(
foo
, {root: 'concurrent'}) - rerender(
bar
, () => { - expect(container).toHaveTextContent('bar') - }) + rerender(
bar
) + expect(container).toHaveTextContent('foo') }) diff --git a/src/pure.js b/src/pure.js index 89488e60..3d15a375 100644 --- a/src/pure.js +++ b/src/pure.js @@ -79,20 +79,22 @@ function render( el.forEach(e => console.log(prettyDOM(e))) : // eslint-disable-next-line no-console, console.log(prettyDOM(el)), - unmount: callback => { + unmount: () => { if (reactRoot) { - reactRoot.unmount(callback) + act(() => { + reactRoot.unmount() + }) } else { ReactDOM.unmountComponentAtNode(container) } }, - rerender: (rerenderUi, callback) => { + rerender: rerenderUi => { if (reactRoot === null) { render(wrapUiIfNeeded(rerenderUi), {container, baseElement}) // Intentionally do not return anything to avoid unnecessarily complicating the API. // folks can use all the same utilities we return in the first place that are bound to the container } else { - reactRoot.render(wrapUiIfNeeded(rerenderUi), callback) + reactRoot.render(wrapUiIfNeeded(rerenderUi)) } }, asFragment: () => { From d552d02a365ddb7b19b474508bcc76cec280c001 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 25 Oct 2019 17:33:26 +0200 Subject: [PATCH 4/5] cleanup should unmount in sync as well --- src/__tests__/concurrent.js | 10 +++++++++- src/pure.js | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/__tests__/concurrent.js b/src/__tests__/concurrent.js index 41e9be83..9c6a7f52 100644 --- a/src/__tests__/concurrent.js +++ b/src/__tests__/concurrent.js @@ -1,5 +1,5 @@ import React from 'react' -import {render} from '../' +import {render, cleanup} from '../' test('simple render works like legacy', () => { const {container} = render(
test
, {root: 'concurrent'}) @@ -22,3 +22,11 @@ test('rerender are flushed in sync', () => { expect(container).toHaveTextContent('foo') }) + +test('cleanup unmounts in sync', () => { + const {container} = render(
test
, {root: 'concurrent'}) + + cleanup() + + expect(container.children).toHaveLength(0) +}) diff --git a/src/pure.js b/src/pure.js index 3d15a375..9583f8ab 100644 --- a/src/pure.js +++ b/src/pure.js @@ -124,7 +124,9 @@ function cleanupAtContainer(container) { if (reactRoot === undefined) { ReactDOM.unmountComponentAtNode(container) } else { - reactRoot.unmount() + act(() => { + reactRoot.unmount() + }) } if (container.parentNode === document.body) { From 31d6edc10fb39c3bf4d90f372d3f27094f4fa340 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 25 Oct 2019 17:54:45 +0200 Subject: [PATCH 5/5] Add test for state updates --- src/__tests__/concurrent.js | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/__tests__/concurrent.js b/src/__tests__/concurrent.js index 9c6a7f52..a70dd237 100644 --- a/src/__tests__/concurrent.js +++ b/src/__tests__/concurrent.js @@ -1,5 +1,5 @@ import React from 'react' -import {render, cleanup} from '../' +import {act, render, cleanup} from '../' test('simple render works like legacy', () => { const {container} = render(
test
, {root: 'concurrent'}) @@ -30,3 +30,23 @@ test('cleanup unmounts in sync', () => { expect(container.children).toHaveLength(0) }) + +test('state updates are concurrent', () => { + function TrackingButton() { + const [clickCount, increment] = React.useReducer(n => n + 1, 0) + + return ( + + ) + } + const {getByRole} = render(, {root: 'concurrent'}) + + act(() => { + getByRole('button').click() + expect(getByRole('button')).toHaveTextContent('Clicked 0 times') + }) + + expect(getByRole('button')).toHaveTextContent('Clicked 1 times') +})