This pull request is automatically built and testable inCodeSandbox.

To see build info of the built libraries, clickhere or the icon next to each commit SHA.

Latest deployment of this branch, based on commit81bd3f6:

Codecov Report

Merging#1214 (f676301) intoalpha (c04b8f0) willnot change coverage.
The diff coverage is100.00%.

@@            Coverage Diff            @@##             alpha     #1214   +/-   ##=========================================  Coverage   100.00%   100.00%           =========================================  Files            4         4             Lines          192       155   -37       Branches        38        30    -8     =========================================- Hits           192       155   -37

Flags with carried forward coverage won't be shown.Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times.Learn more

@eps1lon any chance for an explainer on this one please? AFAIR,async act exists for a few years now, why do we need toawait it now? Thanks!

I always knew this day would come 😅 Everything we do in tests really should be async.

There's a reason why we should do this now and a React specific reason that why we may need to do it in the future.

For now consider this test:

functionApp(){const[ctr,setCtr]=React.useState(0);asyncfunctionsomeAsyncFunction(){// queue a promises to be sure they all flushawaitnull;setCtr(1);}React.useEffect(()=>{someAsyncFunction();},[]);returnctr;}act(()=>{render(<App/>,container);});expect(container.innerHTML).toEqual("1");

based onhttps://github.com/facebook/react/blob/a21d1475ffd7225a463f2d0c0c9b732c8dd795eb/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js#L476-L493

That test will log missing act warnings unless you unmount the treebefore you exit your test i.e. we'd have to tell people to unmount in each test instead of relying on a singleafterEach(cleanup). That's a footgun we can avoid by requiringawait act().

I'm testing this internally first to see how it behaves across Native and Web and to check we cover most scenarios with a codemod (because that's required to land this now). Once that's done I write out a full explainer.

I'm not sure that example is a good justification honestly. Normally if you're setting state there is some visual indication of what state you just set and I would recommend people to make an assertion based on the UI that changed from that state update which would typically result in requiring a query which is wrapped in act. Can you show me a example where that wouldn't be a solution?

I'll add that not always will you have some visual indication that some state changed, but the alternative to a visual indication is typically a function call which can be mocked and the assertion can be inside a waitFor which is also wrapped in act.

We already don't let people assert between what was committed from render and what was committed fromuseEffect. If that is what we want, we need to go back and redesign act. For now, the "missing act" warning is a huge turnoff we need a solution for that scales.

True, but when we're talking about something happening asynchronously (even if triggered by useEffect) that is currently possible and understandable. I'm not convinced this is justified.

True, but when we're talking about something happening asynchronously (even if triggered by useEffect) that is currently possible and understandable. I'm not convinced this is justified.

Just to see I understood, what we're talking about for the example@eps1lon posted is something like this?

functionApp(){const[ctr,setCtr]=React.useState(0);asyncfunctionsomeAsyncFunction(){// queue a promises to be sure they all flushawaitnull;setCtr(1);}React.useEffect(()=>{someAsyncFunction();},[]);returnctr;}test("Should show 1",()=>{act(()=>{render(<App/>,container);});awaitwaitFor(()=>{expect(container.innerHTML).toEqual("1");})})

Because this seems to me like a reasonable usage ofwaitFor, the problem is that people assume they won't need to because we're wrapping all interactions (including render and unmount) withact that should flush all effects.

I don't feel like I am really eligible to provide very valuable feedback. But I found this PR trying to wrap my head better aroundact() especially in combination with Promises. We just switched from CRA to Vite and had to add Jest manually (with RTL), while we did we encountered a ton of new bugs/warnings in tests (not in the actual code). Previously we didn't really need to useact(), but now we have to use it everywhere on user interactions such asUserEvent.click(), UserEvent.type etc.

Debugging and understanding these errors is by far the biggest problem I see with testing react apps. They're always really difficult to debug, understand and solve. Sometimes the act-warnings also only appear when you are running multiple tests at once in parallel which makes it all next to impossible.

TLDR:
I like the idea of enforcing correct usage of act, because I'm struggling today.
I encounter theact(...) warnings sometimes when tests run in parallel, but rerunning the test does not always guarantee the warning will appear again. I.e. the warnings feel inconsistent.

Edit: Clarifications

@eps1lon do we want to move on with this by pushing it to an alpha branch?

Agreed, that would also give us time to update DTL or our supported Node versions before a major release (seetesting-library/dom-testing-library#1255).

I have a bit of time start of October where I want to revisit this. So testing how good codemod coverage is. Testing react-dom and react-native tests. Checking if we want this in React core and then we advance to alpha.

I don't see this blocking Node.js 18 bump. But I would also not force the Node.js 18 bump unless it is necessary or enables some new feature.

But I would also not force the Node.js 18 bump unless it is necessary or enables some new feature.

It's necessary for security.Node 16 is intentionally ending support early because its version of OpenSSL is no longer supported, so we should update our Node versions as soon as we can. Fortunately we're already making progress onDTL's major release dropping Node 16 (currently in alpha), so I'd also like to start migrating RTL and our other libraries to use DTL 10 with Node 18+.

If this PR isn't ready by then, I'd still support releasing in another major. By then I might want to make some changes to support#1209 anyway.

@eps1lon do we want to move on with this by pushing it to an alpha branch?

I'm going to be recording testing videos for EpicWeb.dev starting tomorrow and I would love to use the alpha for these videos to make sure I'm teaching what will be available when EpicWeb.dev is launched. So yeah, an alpha release would be great for me personally :)

@kentcdodds Sounds like a good idea! I suggest we continue working on the other breaking changes for RTL (portingtesting-library/dom-testing-library#1255 and then updating the dependency on DTL) inalpha (we're already using that channel in DTL).@eps1lon can work on this when he's ready, but if@kentcdodds needs a build sooner and the other breaking changes aren't ready, we can merge this into another release branch.

@eps1lon, what are the chances this will actually land in React Testing Library do you think?

I'll be recording videos tomorrow that I would love to not have to re-record. I can use smoke and mirrors to record these videos with asyncrender and stuff, but if you're uncertain this will land by October then maybe I should not do that. So yeah, what do you think?

On my end, like I said I've always felt like this was inevitable we'd end up doing this anyway, so I'm in favor.

hi@eps1lon! are there any updates for the status of this PR? I would love to contribute if needed. It looks like this is blocking people from updating to react 19 and there is no clear direction on what needs to be done in the meantime.

@eps1lon any progress on this? Users of React Native TL reported the same issue (callstack/react-native-testing-library#1787) and I wanted to coordinate the API with the RTL.

Seems like introducing asyncrender,fireEvent, etc is the technically sound solution. That would be a breaking change, and it's only needed if tested components use suspense. What do you think about addingrenderAsync,fireEventAsync etc APIs for such cases, and leaving the original sync cases as is?