Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support async act #10

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: support async act #10

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61faa9f
fix: support async act
MatanBobi Dec 17, 2024
6c9238b
test: add test to check for suspended component
MatanBobi Dec 17, 2024
8716744
rename test
MatanBobi Dec 17, 2024
9999e1e
fix component to suspend
MatanBobi Dec 22, 2024
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
28 changes: 14 additions & 14 deletions src/pure.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ import ReactDOMClient from 'react-dom/client'
// we call act only when rendering to flush any possible effects
// usually the async nature of Vitest browser mode ensures consistency,
// but rendering is sync and controlled by React directly
function act(cb: () => unknown) {
async function act(cb: () => unknown) {
// @ts-expect-error unstable_act is not typed, but exported
const _act = React.act || React.unstable_act
if (typeof _act !== 'function') {
cb()
}
else {
(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true
_act(cb)
await _act(cb)
;(globalThis as any).IS_REACT_ACT_ENVIRONMENT = false
}
}
Expand All @@ -28,8 +28,8 @@ export interface RenderResult extends LocatorSelectors {
maxLength?: number,
options?: PrettyDOMOptions
) => void
unmount: () => void
rerender: (ui: React.ReactNode) => void
unmount: () => Promise<void>
rerender: (ui: React.ReactNode) => Promise<void>
asFragment: () => DocumentFragment
}

Expand All @@ -47,10 +47,10 @@ const mountedRootEntries: {
root: ReturnType<typeof createConcurrentRoot>
}[] = []

export function render(
export async function render(
ui: React.ReactNode,
{ container, baseElement, wrapper: WrapperComponent }: ComponentRenderOptions = {},
): RenderResult {
): Promise<RenderResult> {
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
Expand Down Expand Up @@ -83,7 +83,7 @@ export function render(
})
}

act(() => {
await act(() => {
root!.render(
strictModeIfNeeded(wrapUiIfNeeded(ui, WrapperComponent)),
)
Expand All @@ -93,13 +93,13 @@ export function render(
container,
baseElement,
debug: (el, maxLength, options) => debug(el, maxLength, options),
unmount: () => {
act(() => {
unmount: async () => {
await act(() => {
root.unmount()
})
},
rerender: (newUi: React.ReactNode) => {
act(() => {
rerender: async (newUi: React.ReactNode) => {
await act(() => {
root.render(
strictModeIfNeeded(wrapUiIfNeeded(newUi, WrapperComponent)),
)
Expand All @@ -112,9 +112,9 @@ export function render(
}
}

export function cleanup(): void {
mountedRootEntries.forEach(({ root, container }) => {
act(() => {
export async function cleanup(): Promise<void> {
mountedRootEntries.forEach(async ({ root, container }) => {
await act(() => {
root.unmount()
})
if (container.parentNode === document.body) {
Expand Down
8 changes: 6 additions & 2 deletions test/fixtures/HelloWorld.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export function HelloWorld(): React.ReactElement {
return <div>Hello World</div>
export function HelloWorld({
name = 'World',
}: {
name?: string
}): React.ReactElement {
return <div>{`Hello ${name}`}</div>
}
14 changes: 12 additions & 2 deletions test/render.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,29 @@
import { Suspense } from 'react'
import { expect, test } from 'vitest'
import { page } from '@vitest/browser/context'
import { render } from '../src/index'
import { HelloWorld } from './fixtures/HelloWorld'
import { Counter } from './fixtures/Counter'

test('renders simple component', async () => {
const screen = render(<HelloWorld />)
const screen = await render(<HelloWorld />)
await expect.element(page.getByText('Hello World')).toBeVisible()
expect(screen.container).toMatchSnapshot()
})

test('renders counter', async () => {
const screen = render(<Counter initialCount={1} />)
const screen = await render(<Counter initialCount={1} />)

await expect.element(screen.getByText('Count is 1')).toBeVisible()
await screen.getByRole('button', { name: 'Increment' }).click()
await expect.element(screen.getByText('Count is 2')).toBeVisible()
})

test('renders a suspended component', async () => {
const { getByText } = await render(<HelloWorld name="Vitest" />, {
Copy link

@a-type a-type Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This component won't trigger the suspense boundary since it doesn't throw a promise / use. Testing this behavior will need a specific component implementation. A positive assertion of the suspense boundary being present should help ensure the test is valid.

If React 19 isn't ready to be used in this project yet, maybe something like this?

test('renders a suspended component', async () => {
  let resolve: (() => void) | undefined = undefined;
  const promise = new Promise((res) => {
    resolve = res;
  });
  function TestComponent() {
    throw promise;
    return <div>Hello world</div>;
  }
  const { getByText } = await render(<TestComponent />, ... );
  await expect.element(getByText('Suspended!')).toBeInTheDocument();
  resolve?.();
  await expect.element(getByText('Hello world')).toBeInTheDocument();

I think React somehow resumes execution if the thrown promise is already resolved, but if not a boolean flag could be added to gate the throw.

Copy link
Contributor Author

@MatanBobi MatanBobi Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback.
Two things:

  1. AFAIU, what we're trying to see here is that we're not seeing the Suspense fallback and it has already flushed as part of mounting (that's why we're wrapping the render function). The use case you're mentioning should be covered by the asynchronous nature of the locators. This test is from a reproduction in the issue attached :) I'll rename the test case to make that clear.
  2. The change in this PR is still wanted even if it doesn't satisfy suspended components. act should be awaited and the sync version of act will be deprecated at some point (per the act docs).

Copy link

@a-type a-type Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you're referring to #10, that was my reproduction! My point is mainly that if the component doesn't actually suspend, the boundary will not have been rendered in the first place. I have tested non-suspending components wrapped in suspense boundaries and they work without problems as-is; it's only suspending components which present an issue.

You'll see in the reproduction that the HelloWorld component implementation was altered to suspend via use of a promise: https://github.com/a-type/vitest-browser-react-suspense-repro/blob/main/vitest-example/HelloWorld.tsx#L14

That said, I only wanted to point out that the test wasn't verifying the behavior in question. I don't have an opinion on whether a test is necessary for that behavior, and I'm not a contributor / maintainer anyway!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @a-type :) That's my bad, I totally missed the purpose of the test. Let me have a look and try to implement that.

That said, I only wanted to point out that the test wasn't verifying the behavior in question. I don't have an opinion on whether a test is necessary for that behavior, and I'm not a contributor / maintainer anyway!

Your point is valid and I try to take any comment seriously even if it's not from maintainers :)

Copy link
Contributor Author

@MatanBobi MatanBobi Dec 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@a-type I pushed a fix though I'm hesitating a bit. I wasn't able to figure out yet if the suspense behavior is consistent. In some cases - React will not show the fallback and just show the content (it looks like that depends on how much time we suspend - as can be seen in the failure in CI. If we extend the timeout - the test will pass). I'm trying to figure that out but I don't have a lot of free time right now :)

wrapper: ({ children }) => (
<Suspense fallback={<div>Suspended!</div>}>{children}</Suspense>
),
})
await expect.element(getByText('Hello Vitest')).toBeInTheDocument()
})
Loading