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.

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

Umbrella: act #15472

Closed
acdlite opened this issue Apr 22, 2019 · 9 comments

Comments

@acdlite
Copy link
Member

commented Apr 22, 2019

Action items

  • Before waiting for microtasks to flush, React should call Scheduler.flushAll to flush pending Scheduler work. #15591
  • act should not flush anything until the outermost act call exits (except for the updates that always flush early like flushSync and serial events). #15682
  • Add act warning to React DOM's root.update() (since createRoot is a new API). #15756
  • React should warn if an update is scheduled but is nested inside the wrong renderer's act (e.g. a DOM update nested inside Test Renderer's act), regardless of whether the update was triggered by a legacy API. #15756
  • React should warn if a passive effect is scheduled by an update outside of act, regardless of whether the update was triggered by a legacy API (e.g. this.setState or ReactDOM.render) #15763
  • nested acts from different renderers should work (eg - a react-art update inside a react-dom tree shouldn't warn #15816
  • act should force pending fallbacks to commit at the end, ignoring how much time has passed, without affecting unrelated timers.
  • act should warn if it's called from inside a React event handler or React effect/lifecycle.
  • act should have the same behavior regardless of whether the result is awaited.

Discussion

  • In Legacy Mode, updates that happen after the first await will not be batched, but they shouldn't fire the warning. We should still wait to flush passive effects, Scheduler, and microtasks until the end.
  • Because passive effects are scheduled with Scheduler, they are flushed by Scheduler.flushAll. That means we don't need to call flushPassiveEffects separately in order to flush them. However, we currently use the return value of flushPassiveEffects to determine if additional passive effects were scheduled. So perhaps we should export a method like hasPendingEffects instead.
  • The recommendation is to await the result of act even if the handler is synchronous, because that ensures that any dangling microtasks are flushed before the test proceeds. However, it's hard to fire a warning if the user neglects to do this, because such a warning needs to happen in an async task, and the test could exit before the async task fires. The warning is also controversial because of the additional boilerplate. But regardless of whether we fire a warning, we should stick to our recommendation to always await act.
  • The API is designed primarily for Batched/Concurrent Mode. That's why we wait until the outermost act exits before flushing anything.
    • The behavior is slightly different in Legacy Mode, but they are the same in the simple case of a single event handler inside a single act. For the remaining cases, our suggestion is to switch to the Batched Mode API.
  • No longer need to count the act "depth" because nested acts are a no-op in Batched Mode.

Idiomatic examples

Single event handler

await act(() => setState());

Using a testing framework

await simulate('click', domElement);

where simulate is imported from a testing framework and looks something like:

async function simulate(eventType, domElement) {
  const event = new Event(eventType);
  await act(() => domElement.dispatchEvent(event));
}

Advanced: Multiple events that occur in sequence

In Batched Mode, these would all be flushed in a single batch, so we group them together with an outer act.

await act(async () => {
  await simulate(domElement, 'mousedown');
  await simulate(domElement, 'mouseup');
});
@threepointone

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Here's a scenario that gets harder with the flushAll() approach.

Let's take a component, and test it

function sleep(period) {
  return new Promise(resolve => setTimeout(resolve, period));
}

function App() {
  let [counter, setCounter] = useState(0);
  async function tick() {
    await sleep(500);
    setCounter(x => x + 1);
  }
  useEffect(() => {
    tick();
  }, [Math.min(counter, 5)]);
  return counter === 5 ? <div id="target" /> : null;
}

So, this component waits a half second, increments a counter, 5 times in total. On the final tick, our target element pops into view. With our current setup, we can test it like so -

const dom = document.createElement("div");
document.body.appendChild(dom);
await act(async () => {
  render(<App />, dom);
});

await act(async () => {
  await sleep(2500)
  expect(document.getElementById('target')).toExist();
});

This looks nice. It's a bit of cheating, because the initial render fires off the chain reaction, and we open up another act() scope and keep it open till we know we have what we want.

Now assume effects only flush with Scheduler.flushAll()

await act(() => {
  render(<App />, dom);
});

At this point, we've flushed effects once, but need to somehow

  • flush effects every half second
  • capture every setState inside an act() call
  • stop when the target element pops up

What does that code look like?

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

This test is simpler with a mutationobserver based helper like react-testing-library's render and waitForElement, which are wrapped with act(), so the test looks like

render(<App />, dom);
expect(await waitForElement('#target')).toExist()

But this approach has the same problem as above, since effects/updates wouldn't be getting flushed while it waits.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

another thing - do you think we should warn on nested act()s now? maybe we should just disallow it else the behaviour isn't super intuitive

@threepointone threepointone referenced this issue May 11, 2019
4 of 4 tasks complete
@threepointone

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I have PRs up for all the critical ones. Punting on the last 2 - one of those warnings isn't critical, and I'm not fully convinced for making awaiting an act() mandatory. Let's discuss in our secret super villain lair.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

added an item for making nested acts from different renderers to work.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

going to close this issue since I'm tracking it elsewhere.

@leoselig

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@threepointone Where is "elsewhere"? Since it seems to be the blocking item for 16.9.0 I'd like to follow progress.

@threepointone

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

All the 16.9.0 items are done, the remaining are for the future.

@gaearon

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

@leoselig You can try 0.0.0-db3ae32b8 which is likely going to be a release candidate for 16.9.0.

robertknight added a commit to preactjs/preact that referenced this issue Aug 11, 2019
Implement support for nested calls to `act`
When calls to act are nested, effects and component updates are only
flushed when the outer `act` call returns, as per [1] and [2].

This is convenient for creating helper functions which may invoke `act`
themselves.

[1] facebook/react#15682
[2] facebook/react#15472
robertknight added a commit to preactjs/preact that referenced this issue Aug 11, 2019
Implement support for nested calls to `act`
When calls to act are nested, effects and component updates are only
flushed when the outer `act` call returns, as per [1] and [2].

This is convenient for creating helper functions which may invoke `act`
themselves.

[1] facebook/react#15682
[2] facebook/react#15472
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.