fix: Require await act(...)

[eps1lon]

This is a WIP. Changes are described as they are discovered.

Codemod: https://github.com/eps1lon/codemod-missing-await-act

fixes to missing act warnings

Sometimes you wanted to assert on a given commit of the component without caring about scheduled tasks. This always worked since these scheduled tasks were on the macrotask queue (e.g. setTimeout) because we automatically unmounted the component after the test ended.
However, when these tasks where on the microtasks queue (e.g. a promise that just resolved), you would get a missing act warning between the test ending and our automatic cleanup running.

The only solution would've been to add an explicit unmount to each test.

Now that we require await act(...) these issues will no longer persist since we'll also flush the state updates on the microtask queue. If you want to continue asserting on the intermediate states, you can use globalThis.IS_REACT_ACT_ENVIRONMENT = false. Keep in mind though that while this flag is set, you have to manually wait for every state update to be applied e.g. by using findBy or waitFor.

Timer changes after module initialization

Drops support for changing timers after module initialization.
This affects tests that changed the timer implementation (i.e. fake vs real timers) after modules were imported. For example, in the following test, updates are no longer flushed.

import { render, screen } from "@testing-library/react";
import { useState } from "react";

jest.useFakeTimers;

test("updates", async () => {
  function Component() {
    const [state, setState] = useState("initial");
    useEffect(() => {
      const timeoutID = setTimeout(() => {
        setState("committed");
      }, 50);

      return () => {
        clearTimeout(timeoutID);
      };
    }, []);
    return state;
  }
  const { container } = await render(<Component />);

  // will timeout
  await waitFor(() => {
    expect(container.textContent).toEqual("committed");
  });
});

Instead, you have to decide on timers before you import modules. In Jest, you can choose timers at the config level, or keep using jest.useFaketimers() but call it before any import or require.

This only ever worked because we cheated a bit and flipped silently to an act environment during waitFor. However, since we now flush a bit more when using IS_REACT_ACT_ENVIRONMENT, we need to offer APIs to assert on intermediate states e.g. when your component initially displays a loading spinner that goes away in the next microtask. You can do this by simply setting globalThis.IS_REACT_ACT_ENVIRONMENT = false and continue to use existing APIs. React will fallback to standard (real or fake) timers and not the act queue.

Full explainer will follow. Change will be accompanied by a codemod: https://github.com/eps1lon/codemod-missing-await-act

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

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

Latest deployment of this branch, based on commit dd27bd9:

Sandbox	Source
react-testing-library-examples	Configuration

[codecov]

Codecov Report

Merging #1214 (f676301) into alpha (c04b8f0) will not change coverage.
The diff coverage is 100.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     

Flag	Coverage Δ
canary	100.00% <100.00%> (ø)
experimental	100.00% <100.00%> (ø)
latest	100.00% <100.00%> (ø)

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

Files	Coverage Δ
src/act-compat.js	100.00% <100.00%> (ø)
src/fire-event.js	100.00% <100.00%> (ø)
src/index.js	100.00% <100.00%> (ø)
src/pure.js	100.00% <100.00%> (ø)

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

[MatanBobi]

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

[kentcdodds]

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

[eps1lon]

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:

function App() {
  const [ctr, setCtr] = React.useState(0);
  async function someAsyncFunction() {
    // queue a promises to be sure they all flush
    await null;
    setCtr(1);
  }
  React.useEffect(() => {
    someAsyncFunction();
  }, []);
  return ctr;
}
act(() => {
  render(<App />, container);
});
expect(container.innerHTML).toEqual("1");

based on https://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 tree before you exit your test i.e. we'd have to tell people to unmount in each test instead of relying on a single afterEach(cleanup). That's a footgun we can avoid by requiring await 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.

[kentcdodds]

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?

[kentcdodds]

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.

[eps1lon]

We already don't let people assert between what was committed from render and what was committed from useEffect. 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.

[kentcdodds]

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.

[MatanBobi]

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?

function App() {
  const [ctr, setCtr] = React.useState(0);
  async function someAsyncFunction() {
    // queue a promises to be sure they all flush
    await null;
    setCtr(1);
  }
  React.useEffect(() => {
    someAsyncFunction();
  }, []);
  return ctr;
}

test("Should show 1", () => {
  act(() => {
    render(<App />, container);
  });
  await waitFor(() => {
    expect(container.innerHTML).toEqual("1");
  })
})

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

[SimpleCookie]

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 around act() 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 use act(), but now we have to use it everywhere on user interactions such as UserEvent.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 the act(...) 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

[MatanBobi]

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

[nickserv]

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

[eps1lon]

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.

[nickserv]

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 on DTL'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.

[kentcdodds]

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

[nickserv]

@kentcdodds Sounds like a good idea! I suggest we continue working on the other breaking changes for RTL (porting testing-library/dom-testing-library#1255 and then updating the dependency on DTL) in alpha (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.

[alexkrolick]

FWIW, making render async and wrapping it with act() by default is also the solution I came up with in my current project (on React 17) to get stable and consistent first assertions in tests

[kentcdodds]

@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 async render 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.

[eps1lon]

Add comment here and to src/pure.js explaining why export order is important.

iirc we had a discussion around this with Rollup people and this was actually specified behavior: first export wins not last like you would expect if it would work like object spreading i.e. export * from ...; export { fireEvent } is not equivalent to module.exports = { ...dtlExports, fireEvent } Read next comment to understand which export is used.

[eps1lon]

Found it: #790 (comment)

TL;DR: By spec, the local fireEvent should be preferred. Babel specifically transpiles the export * so that the local fireEvent is in exports not the one from @testing-library/dom.

So this is a TS quirk we have to work around. I try to create a repro to confirm I didn't hallucinate.

[eps1lon]

Reverting for now. Local checkout was pointing to the fireEvent from /dom but this may have been the language server having an outdated install. Repros do match spec behavior.

[eps1lon]

Check if we added a test for this.