From c888cb6e7afab8719ba385838a9323237357d514 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Mon, 13 Sep 2021 10:49:05 +0200 Subject: [PATCH] feat: Use concurrent React when available (#937) BREAKING CHANGE: If you have React 18 installed, we'll use the new [`createRoot` API](https://github.com/reactwg/react-18/discussions/5) by default which comes with a set of [changes while also enabling support for concurrent features](https://github.com/reactwg/react-18/discussions/4). To can opt-out of this change by using `render(ui, { legacyRoot: true } )`. But be aware that the legacy root API is deprecated in React 18 and its usage will trigger console warnings. --- .github/workflows/validate.yml | 3 + jest.config.js | 15 +++ package.json | 2 +- src/__tests__/cleanup.js | 19 +--- src/__tests__/end-to-end.js | 2 + src/__tests__/new-act.js | 6 +- src/__tests__/no-act.js | 8 ++ src/__tests__/old-act.js | 6 +- src/__tests__/render.js | 33 +++++++ src/__tests__/stopwatch.js | 5 +- src/act-compat.js | 9 +- src/pure.js | 161 ++++++++++++++++++++++++++------- tests/setup-env.js | 19 ---- types/index.d.ts | 5 + 14 files changed, 210 insertions(+), 83 deletions(-) create mode 100644 jest.config.js diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 67b71c24..45cc7d13 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -16,6 +16,7 @@ jobs: # ignore all-contributors PRs if: ${{ !contains(github.head_ref, 'all-contributors') }} strategy: + fail-fast: false matrix: # TODO: relax `'16.9.1'` to `16` once GitHub has 16.9.1 cached. 16.9.0 is broken due to https://github.com/nodejs/node/issues/40030 node: [12, 14, '16.9.1'] @@ -52,6 +53,8 @@ jobs: - name: ⬆️ Upload coverage report uses: codecov/codecov-action@v1 + with: + flags: ${{ matrix.react }} release: needs: main diff --git a/jest.config.js b/jest.config.js new file mode 100644 index 00000000..5c840226 --- /dev/null +++ b/jest.config.js @@ -0,0 +1,15 @@ +const {jest: jestConfig} = require('kcd-scripts/config') + +module.exports = Object.assign(jestConfig, { + coverageThreshold: { + ...jestConfig.coverageThreshold, + // full coverage across the build matrix (React 17, 18) but not in a single job + './src/pure': { + // minimum coverage of jobs using React 17 and 18 + branches: 80, + functions: 78, + lines: 84, + statements: 84, + }, + }, +}) diff --git a/package.json b/package.json index 1cd283fb..cbf61367 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,7 @@ "license": "MIT", "dependencies": { "@babel/runtime": "^7.12.5", - "@testing-library/dom": "^8.0.0" + "@testing-library/dom": "^8.5.0" }, "devDependencies": { "@testing-library/jest-dom": "^5.11.6", diff --git a/src/__tests__/cleanup.js b/src/__tests__/cleanup.js index 9d3f52d4..0dcbac12 100644 --- a/src/__tests__/cleanup.js +++ b/src/__tests__/cleanup.js @@ -83,10 +83,7 @@ describe('fake timers and missing act warnings', () => { expect(microTaskSpy).toHaveBeenCalledTimes(0) // console.error is mocked // eslint-disable-next-line no-console - expect(console.error).toHaveBeenCalledTimes( - // ReactDOM.render is deprecated in React 18 - React.version.startsWith('18') ? 1 : 0, - ) + expect(console.error).toHaveBeenCalledTimes(0) }) test('cleanup does not swallow missing act warnings', () => { @@ -118,16 +115,10 @@ describe('fake timers and missing act warnings', () => { expect(deferredStateUpdateSpy).toHaveBeenCalledTimes(1) // console.error is mocked // eslint-disable-next-line no-console - expect(console.error).toHaveBeenCalledTimes( - // ReactDOM.render is deprecated in React 18 - React.version.startsWith('18') ? 2 : 1, - ) + expect(console.error).toHaveBeenCalledTimes(1) // eslint-disable-next-line no-console - expect( - console.error.mock.calls[ - // ReactDOM.render is deprecated in React 18 - React.version.startsWith('18') ? 1 : 0 - ][0], - ).toMatch('a test was not wrapped in act(...)') + expect(console.error.mock.calls[0][0]).toMatch( + 'a test was not wrapped in act(...)', + ) }) }) diff --git a/src/__tests__/end-to-end.js b/src/__tests__/end-to-end.js index cf222aec..787a944d 100644 --- a/src/__tests__/end-to-end.js +++ b/src/__tests__/end-to-end.js @@ -17,6 +17,8 @@ function ComponentWithLoader() { let cancelled = false fetchAMessage().then(data => { if (!cancelled) { + // Will trigger "missing act" warnings in React 18 with real timers + // Need to wait for an action on https://github.com/reactwg/react-18/discussions/23#discussioncomment-1087897 setState({data, loading: false}) } }) diff --git a/src/__tests__/new-act.js b/src/__tests__/new-act.js index af81e29c..42552594 100644 --- a/src/__tests__/new-act.js +++ b/src/__tests__/new-act.js @@ -1,4 +1,4 @@ -let asyncAct, consoleErrorMock +let asyncAct jest.mock('react-dom/test-utils', () => ({ act: cb => { @@ -9,11 +9,11 @@ jest.mock('react-dom/test-utils', () => ({ beforeEach(() => { jest.resetModules() asyncAct = require('../act-compat').asyncAct - consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {}) + jest.spyOn(console, 'error').mockImplementation(() => {}) }) afterEach(() => { - consoleErrorMock.mockRestore() + console.error.mockRestore() }) test('async act works when it does not exist (older versions of react)', async () => { diff --git a/src/__tests__/no-act.js b/src/__tests__/no-act.js index d739e763..de4117bb 100644 --- a/src/__tests__/no-act.js +++ b/src/__tests__/no-act.js @@ -12,7 +12,15 @@ afterEach(() => { consoleErrorMock.mockRestore() }) +// no react-dom/test-utils also means no isomorphic act since isomorphic act got released after test-utils act jest.mock('react-dom/test-utils', () => ({})) +jest.mock('react', () => { + const ReactActual = jest.requireActual('react') + + delete ReactActual.unstable_act + + return ReactActual +}) test('act works even when there is no act from test utils', () => { const callback = jest.fn() diff --git a/src/__tests__/old-act.js b/src/__tests__/old-act.js index 6081fef8..0153fea3 100644 --- a/src/__tests__/old-act.js +++ b/src/__tests__/old-act.js @@ -1,13 +1,13 @@ -let asyncAct, consoleErrorMock +let asyncAct beforeEach(() => { jest.resetModules() asyncAct = require('../act-compat').asyncAct - consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {}) + jest.spyOn(console, 'error').mockImplementation(() => {}) }) afterEach(() => { - consoleErrorMock.mockRestore() + console.error.mockRestore() }) jest.mock('react-dom/test-utils', () => ({ diff --git a/src/__tests__/render.js b/src/__tests__/render.js index fea1a649..ac996444 100644 --- a/src/__tests__/render.js +++ b/src/__tests__/render.js @@ -101,3 +101,36 @@ test('flushes useEffect cleanup functions sync on unmount()', () => { expect(spy).toHaveBeenCalledTimes(1) }) + +test('throws if `legacyRoot: false` is used with an incomaptible version', () => { + const isConcurrentReact = typeof ReactDOM.createRoot === 'function' + + const performConcurrentRender = () => render(
, {legacyRoot: false}) + + // eslint-disable-next-line jest/no-if -- jest doesn't support conditional tests + if (isConcurrentReact) { + // eslint-disable-next-line jest/no-conditional-expect -- yes, jest still doesn't support conditional tests + expect(performConcurrentRender).not.toThrow() + } else { + // eslint-disable-next-line jest/no-conditional-expect -- yes, jest still doesn't support conditional tests + expect(performConcurrentRender).toThrowError( + `Attempted to use concurrent React with \`react-dom@${ReactDOM.version}\`. Be sure to use the \`next\` or \`experimental\` release channel (https://reactjs.org/docs/release-channels.html).`, + ) + } +}) + +test('can be called multiple times on the same container', () => { + const container = document.createElement('div') + + const {unmount} = render(, {container}) + + expect(container).toContainHTML('') + + render(, {container}) + + expect(container).toContainHTML('') + + unmount() + + expect(container).toBeEmptyDOMElement() +}) diff --git a/src/__tests__/stopwatch.js b/src/__tests__/stopwatch.js index 400fce10..eeaf395c 100644 --- a/src/__tests__/stopwatch.js +++ b/src/__tests__/stopwatch.js @@ -53,8 +53,5 @@ test('unmounts a component', async () => { // and get an error. await sleep(5) // eslint-disable-next-line no-console - expect(console.error).toHaveBeenCalledTimes( - // ReactDOM.render is deprecated in React 18 - React.version.startsWith('18') ? 1 : 0, - ) + expect(console.error).not.toHaveBeenCalled() }) diff --git a/src/act-compat.js b/src/act-compat.js index 40ecdab9..16124afc 100644 --- a/src/act-compat.js +++ b/src/act-compat.js @@ -2,8 +2,9 @@ import * as React from 'react' import ReactDOM from 'react-dom' import * as testUtils from 'react-dom/test-utils' -const reactAct = testUtils.act -const actSupported = reactAct !== undefined +const isomorphicAct = React.unstable_act +const domAct = testUtils.act +const actSupported = domAct !== undefined // act is supported react-dom@16.8.0 // so for versions that don't have act from test utils @@ -14,7 +15,7 @@ function actPolyfill(cb) { ReactDOM.render(
, document.createElement('div')) } -const act = reactAct || actPolyfill +const act = isomorphicAct || domAct || actPolyfill let youHaveBeenWarned = false let isAsyncActSupported = null @@ -50,7 +51,7 @@ function asyncAct(cb) { } let cbReturn, result try { - result = reactAct(() => { + result = domAct(() => { cbReturn = cb() return cbReturn }) diff --git a/src/pure.js b/src/pure.js index 75098f78..0ac63feb 100644 --- a/src/pure.js +++ b/src/pure.js @@ -25,32 +25,73 @@ configureDTL({ }, }) +if (React.startTransition !== undefined) { + configureDTL({ + unstable_advanceTimersWrapper: cb => { + return act(cb) + }, + asyncWrapper: cb => cb(), + }) +} + +// Ideally we'd just use a WeakMap where containers are keys and roots are values. +// We use two variables so that we can bail out in constant time when we render with a new container (most common use case) +/** + * @type {Set} + */ const mountedContainers = new Set() +/** + * @type Array<{container: import('react-dom').Container, root: ReturnType}> + */ +const mountedRootEntries = [] -function render( - ui, - { - container, - baseElement = container, - queries, - hydrate = false, - wrapper: WrapperComponent, - } = {}, -) { - if (!baseElement) { - // default to document.body instead of documentElement to avoid output of potentially-large - // head elements (such as JSS style blocks) in debug output - baseElement = document.body +function createConcurrentRoot(container, options) { + if (typeof ReactDOM.createRoot !== 'function') { + throw new TypeError( + `Attempted to use concurrent React with \`react-dom@${ReactDOM.version}\`. Be sure to use the \`next\` or \`experimental\` release channel (https://reactjs.org/docs/release-channels.html).'`, + ) } - if (!container) { - container = baseElement.appendChild(document.createElement('div')) + const root = options.hydrate + ? ReactDOM.hydrateRoot(container) + : ReactDOM.createRoot(container) + + return { + hydrate(element) { + /* istanbul ignore if */ + if (!options.hydrate) { + throw new Error( + 'Attempted to hydrate a non-hydrateable root. This is a bug in `@testing-library/react`.', + ) + } + root.render(element) + }, + render(element) { + root.render(element) + }, + unmount() { + root.unmount() + }, } +} - // we'll add it to the mounted containers regardless of whether it's actually - // added to document.body so the cleanup method works regardless of whether - // they're passing us a custom container or not. - mountedContainers.add(container) +function createLegacyRoot(container) { + return { + hydrate(element) { + ReactDOM.hydrate(element, container) + }, + render(element) { + ReactDOM.render(element, container) + }, + unmount() { + ReactDOM.unmountComponentAtNode(container) + }, + } +} +function renderRoot( + ui, + {baseElement, container, hydrate, queries, root, wrapper: WrapperComponent}, +) { const wrapUiIfNeeded = innerElement => WrapperComponent ? React.createElement(WrapperComponent, null, innerElement) @@ -58,9 +99,9 @@ function render( act(() => { if (hydrate) { - ReactDOM.hydrate(wrapUiIfNeeded(ui), container) + root.hydrate(wrapUiIfNeeded(ui), container) } else { - ReactDOM.render(wrapUiIfNeeded(ui), container) + root.render(wrapUiIfNeeded(ui), container) } }) @@ -75,11 +116,15 @@ function render( console.log(prettyDOM(el, maxLength, options)), unmount: () => { act(() => { - ReactDOM.unmountComponentAtNode(container) + root.unmount() }) }, rerender: rerenderUi => { - render(wrapUiIfNeeded(rerenderUi), {container, baseElement}) + renderRoot(wrapUiIfNeeded(rerenderUi), { + container, + baseElement, + root, + }) // 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 }, @@ -99,20 +144,66 @@ function render( } } -function cleanup() { - mountedContainers.forEach(cleanupAtContainer) +function render( + ui, + { + container, + baseElement = container, + legacyRoot = typeof ReactDOM.createRoot !== 'function', + queries, + hydrate = false, + wrapper, + } = {}, +) { + if (!baseElement) { + // default to document.body instead of documentElement to avoid output of potentially-large + // head elements (such as JSS style blocks) in debug output + baseElement = document.body + } + if (!container) { + container = baseElement.appendChild(document.createElement('div')) + } + + let root + // eslint-disable-next-line no-negated-condition -- we want to map the evolution of this over time. The root is created first. Only later is it re-used so we don't want to read the case that happens later first. + if (!mountedContainers.has(container)) { + const createRootImpl = legacyRoot ? createLegacyRoot : createConcurrentRoot + root = createRootImpl(container, {hydrate}) + + mountedRootEntries.push({container, root}) + // we'll add it to the mounted containers regardless of whether it's actually + // added to document.body so the cleanup method works regardless of whether + // they're passing us a custom container or not. + mountedContainers.add(container) + } else { + mountedRootEntries.forEach(rootEntry => { + if (rootEntry.container === container) { + root = rootEntry.root + } + }) + } + + return renderRoot(ui, { + container, + baseElement, + queries, + hydrate, + wrapper, + root, + }) } -// 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) { - act(() => { - ReactDOM.unmountComponentAtNode(container) +function cleanup() { + mountedRootEntries.forEach(({root, container}) => { + act(() => { + root.unmount() + }) + if (container.parentNode === document.body) { + document.body.removeChild(container) + } }) - if (container.parentNode === document.body) { - document.body.removeChild(container) - } - mountedContainers.delete(container) + mountedRootEntries.length = 0 + mountedContainers.clear() } // just re-export everything from dom-testing-library diff --git a/tests/setup-env.js b/tests/setup-env.js index 6c0b953b..264828a9 100644 --- a/tests/setup-env.js +++ b/tests/setup-env.js @@ -1,20 +1 @@ import '@testing-library/jest-dom/extend-expect' - -let consoleErrorMock - -beforeEach(() => { - const originalConsoleError = console.error - consoleErrorMock = jest - .spyOn(console, 'error') - .mockImplementation((message, ...optionalParams) => { - // Ignore ReactDOM.render/ReactDOM.hydrate deprecation warning - if (message.indexOf('Use createRoot instead.') !== -1) { - return - } - originalConsoleError(message, ...optionalParams) - }) -}) - -afterEach(() => { - consoleErrorMock.mockRestore() -}) diff --git a/types/index.d.ts b/types/index.d.ts index 663e6280..b4386996 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -58,6 +58,11 @@ export interface RenderOptions< * @see https://testing-library.com/docs/react-testing-library/api/#hydrate) */ hydrate?: boolean + /** + * Set to `true` if you want to force synchronous `ReactDOM.render`. + * Otherwise `render` will default to concurrent React if available. + */ + legacyRoot?: boolean /** * Queries to bind. Overrides the default set from DOM Testing Library unless merged. *