Skip to content

feat: support async act#10

Closed
MatanBobi wants to merge 4 commits into
vitest-community:mainfrom
MatanBobi:fix/support-async-act
Closed

feat: support async act#10
MatanBobi wants to merge 4 commits into
vitest-community:mainfrom
MatanBobi:fix/support-async-act

Conversation

@MatanBobi

@MatanBobi MatanBobi commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

Resolves #9

Still need to add a test for a suspended component, will work on it soon.
Test added.

Comment thread test/render.test.tsx Outdated
})

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

@a-type a-type Dec 17, 2024

Copy link
Copy Markdown

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.

@MatanBobi MatanBobi Dec 17, 2024

Copy link
Copy Markdown
Contributor Author

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).

@a-type a-type Dec 18, 2024

Copy link
Copy Markdown

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
Copy Markdown
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 :)

@MatanBobi MatanBobi Dec 22, 2024

Copy link
Copy Markdown
Contributor Author

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 :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah, timeout works but isn't ideal for testing. I'd recommend either using fake timers, or a 'gate' boolean that is flipped on the first render and resolves the second one.

Do you want me to contribute here? I have a lot of experience working with Suspense as a library author.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Feel free :)
I didn't fix this yet as I wanted to get a clear answer from the React team on what's the threshold for client side suspense to present the fallback.
I was aiming to use fake timers, just didn't want to guess the number and have it break in next react releases.

@MatanBobi MatanBobi changed the title fix: support async act feat: support async act Dec 17, 2024
@sheremet-va

Copy link
Copy Markdown
Member

Any news on this?

@MatanBobi

Copy link
Copy Markdown
Contributor Author

Sorry @sheremet-va, got busy with some other stuff. Closing this as I see someone else followed up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Components which suspend do not resolve suspense when tested

3 participants