Skip to content
Merged
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .github/workflows/validate.yml
Original file line number Diff line number Diff line change
@@ -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
15 changes: 15 additions & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -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,
},
},
})
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -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",
19 changes: 5 additions & 14 deletions src/__tests__/cleanup.js
Original file line number Diff line number Diff line change
@@ -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(...)',
)
})
})
2 changes: 2 additions & 0 deletions src/__tests__/end-to-end.js
Original file line number Diff line number Diff line change
@@ -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})
}
})
6 changes: 3 additions & 3 deletions src/__tests__/new-act.js
Original file line number Diff line number Diff line change
@@ -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 () => {
8 changes: 8 additions & 0 deletions src/__tests__/no-act.js
Original file line number Diff line number Diff line change
@@ -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()
6 changes: 3 additions & 3 deletions src/__tests__/old-act.js
Original file line number Diff line number Diff line change
@@ -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', () => ({
33 changes: 33 additions & 0 deletions src/__tests__/render.js
Original file line number Diff line number Diff line change
@@ -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(<div />, {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(<strong />, {container})

expect(container).toContainHTML('<strong></strong>')

render(<em />, {container})

expect(container).toContainHTML('<em></em>')

unmount()

expect(container).toBeEmptyDOMElement()
})
5 changes: 1 addition & 4 deletions src/__tests__/stopwatch.js
Original file line number Diff line number Diff line change
@@ -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()
})
9 changes: 5 additions & 4 deletions src/act-compat.js
Original file line number Diff line number Diff line change
@@ -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 [email protected]
// so for versions that don't have act from test utils
@@ -14,7 +15,7 @@ function actPolyfill(cb) {
ReactDOM.render(<div />, 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
})
161 changes: 126 additions & 35 deletions src/pure.js
Original file line number Diff line number Diff line change
@@ -25,42 +25,83 @@ configureDTL({
},
})

if (React.startTransition !== undefined) {
configureDTL({
unstable_advanceTimersWrapper: cb => {
return act(cb)
},
asyncWrapper: cb => cb(),
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary to configure?

Copy link
Member Author

Choose a reason for hiding this comment

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

TL;DR: I misunderstood the question. I think we don't need to configure that because it's the default. I still leave my original long winded explainer for future reference and because it made me realize I still don't fully understand why act() behaves the way it does.

So let's say we have

waitFor(() => expect(getByRole('button')).toBeInTheDocument())

and the button would only appear after two intervals. This would previously translate to

act(() => {
   jest.advanceTimersByTime();
   checkCallback();
   jest.advanceTimersByTime();
   checkCallback();
});

The problem is that React only flushes effects when exiting the act call. So checkCallback could not assert on any effect scheduled due to timers being advanced.
If we just configure unstable_advanceTimersWrapper to use act we would now have

act(() => {
   jest.advanceTimersByTime();
   act(() => {
     checkCallback();
   })
   jest.advanceTimersByTime();
   act(() => {
     checkCallback();
   });
});

The problem now is that React only flushes scheduled effects in the outermost act so we need to remove that.

I think I good question is why only the outermost act is considered.

Copy link
Member Author

@eps1lon eps1lon Sep 11, 2021

Choose a reason for hiding this comment

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

I need to double check something though. React 17 will no longer have act as the asyncWrapper but somehow our React 17 tests are still passing. So either our tests are incomplete or this was dead code.

Copy link
Member

@MatanBobi MatanBobi Sep 11, 2021

Choose a reason for hiding this comment

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

I need to double check something though. React 17 will no longer have act as the asyncWrapper but somehow our React 17 tests are still passing. So either our tests are incomplete or this was dead code.

Did you mean React 18? If so, following this comment I'm not sure it won't work, it's just not needed anymore, isn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean React 18? If so, following this comment I'm not sure it won't work, it's just not needed anymore, isn't it?

No, 17. I was reffering to our usage though. I changed the behavior for React 17 as well but no tests started failing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I need to double check something though. React 17 will no longer have act as the asyncWrapper but somehow our React 17 tests are still passing. So either our tests are incomplete or this was dead code.

Tests are incomplete due to usage of class components. Backported the new tests with a function component in #962 that should ensure asyncWrapper is still present in React 17.

Copy link
Member Author

Choose a reason for hiding this comment

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

Prevented mild disaster since the new tests also uncovered silly lines such as typeof React.startTransition !== undefined. It's either React.startTransition !== undefined or typeof React.startTransition !== 'undefined'.

Also uncovered that the "configure wrapper" approach is not compatible with supporting different timers across multiple versions of react.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to leave this a bit until the next WG meeting. Unclear what the timeline is for work on act. Feels like the core team already moved on but there are still glaring holes in the behavior of act().

Copy link
Member Author

Choose a reason for hiding this comment

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

Though I'm personally fine with shipping this as an alpha to test integration. We haven't solved all problems yet for the alpha but neither has React itself. I'm fine with shipping this in an incomplete state since I can just blame React about all the missing parts 😎

Copy link
Member

Choose a reason for hiding this comment

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

Sounds fine to me 👍

})
}

// 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<import('react-dom').Container>}
*/
const mountedContainers = new Set()
/**
* @type Array<{container: import('react-dom').Container, root: ReturnType<typeof createConcurrentRoot>}>
*/
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)
: innerElement

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
19 changes: 0 additions & 19 deletions tests/setup-env.js
Original file line number Diff line number Diff line change
@@ -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()
})
5 changes: 5 additions & 0 deletions types/index.d.ts
Original file line number Diff line number Diff line change
@@ -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.
*