Planned changes to act testing API

[acdlite]

(spawned from #21 (reply in thread))

There's a lot of planned work that needs to be done before release. What's implemented today isn't how we want it to work in the future.

The current implementation of act is more convoluted than it could be because we originally thought we needed to support act in production mode. But we also didn't want to add act logic to the production React DOM bundle. So we had to implement act using clever user space tricks that don't work exactly as we'd like.

Instead, we decided that act doesn't need to exist in production mode. Production mode is only used for testing in e2e environments, where act isn't useful. (In an e2e environment, you perform an interaction and wait for the DOM to update; no special framework integration required.) act is really designed for integration and unit tests, which in practice everyone runs in development only.

Because act will only exist in development mode, we can add additional logic to the development build of React DOM. What it will do is post React tasks on a special React queue, instead of posting them to the browser like in a real app. Then act can flush that queue directly — and synchronously — without any clever hacks.

The upshot is that the sync version of act will work as you describe. No need for await null to flush promise microtasks because we won't have scheduled a microtask at all. Though it's still useful for cases where you want to simulate an async event handler.

cc: @threepointone with whom we made this decision when he was on our team (and who is the creator of the API) 🙂

Update

I've posted a related proposal here: #102

[eps1lon]

Will we still need to mock the scheduler with scheduler/unstable_mock or will this automatically happen when act() causes tasks to be posted on this special React queue? This would be more than welcome since mocking the scheduler

1. requires extra setup
2. requires different solutions with varying degrees of complexity depending on the target environment (bundled, codesandbox, native ES modules etc)

[acdlite]

Forgot to mention that! Good call out.

No mocking of Scheduler will be required, because when act is detected we will bypass Scheduler entirely in favor of a special, act-only task queue. Then act will flush that queue directly at the end of the event.

(We will still use the Scheduler-mocking approach in the React repo, because our own tests actually do need to run in production mode. But that's a special circumstance of our tests alone — we test React in production so you don't have to!)

[eps1lon]

One thing that's unclear to me is how we're supposed to test timer based updates (e.g. setTimeout(() => setState()) or fetch().then(() => setState())) in tests using real timers.

In React 17 all we had to do was wrap some polling function in act e.g.

await ReactDOMTestUtils.act(async () => {
  await new Promise((resolve) => {
    setInterval(async () => {
      if (
        // That's the condition we wait for
        document.querySelector("main").getAttribute("aria-busy") !== "true"
      ) {
        resolve();
      }
    }, 50);
  });
});

The above code is abstracted away in @testing-library/react with await waitFor(() => document.querySelector("main").getAttribute("aria-busy") !== "true").

In concurrent React the above code times out.

With fake timers there's no change as far as I can tell since we can wrap advanceTime in act (and some additional logic for some edge cases):

ReactDOMTestUtils.act(() => {
  // hardcode the time it take to resolve the mocked fetch
  jest.advanceTimersByTime(100);
});

Full repro: https://github.com/eps1lon/react-concurrent-testing-timers

[eps1lon]

The thinking was that if you do want to use something like waitFor, then you're probably writing an e2e test and don't need to use act — you can just wait for React rendering to flush the way it does in a regular environment.

Makes sense.

Can you explain the flaw in this recommendation?

Not sure to whom this refers. But I would've said that there's still the missing act warning though now you're saying you'll remove that anyway. If that's the case, the I don't see any problem yet.

[acdlite]

Ok that's helpful, I think we probably will go ahead and remove that.

The thinking is that with a concurrent root, you're probably not going to forget to use act because if you don't then nothing will render anyway :D

[eps1lon]

Ok that's helpful, I think we probably will go ahead and remove that.

Yeah, especially because I find this warning not helpful at the moment. It's rather "spammy" (see facebook/react#19416 and facebook/react#19318)

[acdlite]

I don't think facebook/react#19318 is the best example because in a concurrent root, act is not only about passive effects. It flushes the rendering, too.

[acdlite]

Proposal here: #102

[Andarist]

act is really designed for integration and unit tests, which in practice everyone runs in development only.

We have a single (I think) test that executes Emotion (and by extension also React) in production mode:
https://github.com/emotion-js/emotion/blob/4a212581208067992433cf4186aef52c49f2fd4a/packages/react/__tests__/import-prod.js

The test is using RTL and it was always resulting in an act-related error:
https://github.com/emotion-js/emotion/runs/4571155796?check_suite_focus=true#step:7:72
but other than that this was working in the test environment. However, this now throws an error and thus this test fails.

What would be the recommended approach to refactor this? The RTL API is OK and it's cool to just reuse the library here, OTOH it is designed around act, right? Should this test just switch to the barebones DOM Testing Library? cc @eps1lon