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.

Sign up for GitHub

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

Jump to bottom

Move update scheduling to microtask #26512

Merged
merged 3 commits into from
Mar 31, 2023
Merged

Move update scheduling to microtask #26512

merged 3 commits into from
Mar 31, 2023

Commits on Mar 31, 2023

  1. Move didScheduleLegacyUpdate 

    This moves a special check related to `act` in the legacy renderer. I
moved it out of the `ensureRootIsScheduled` path and into
`scheduleUpdateOnFiber`. There's some related logic that's already there
and it makes sense to keep them together.

Pure refactor, no behavior change.
    @acdlite
    acdlite committed Mar 31, 2023
    Configuration menu
    Copy the full SHA
    6be85c3 View commit details
    Browse the repository at this point in the history

  2. Move update processing into microtask 

    When React receives new input (via `setState`, a Suspense promise
resolution, and so on), it needs to ensure there's a rendering task
associated with the update. Most of this happens
`ensureRootIsScheduled`.

If a single event contains multiple updates, we end up running the
scheduling code once per update. But this is wasteful because we really
only need to run it once, at the end of the event (or in the case of
flushSync, at the end of the scope function's execution).

So this PR moves the scheduling logic to happen in a microtask instead.
In some cases, we will force it run earlier than that, like for
`flushSync`, but since updates are batched by default, it will almost
always happen in the microtask. Even for discrete updates.

In production, this should have no observable behavior difference. In
a testing environment that uses `act`, this should also not have a
behavior difference because React will push these tasks to an internal
`act` queue.

However, tests that do not use `act` and do not simulate an actual
production environment (like an e2e test) may be affected. For example,
before this change, if a test were to call `setState` outside of `act`
and then immediately call `jest.runAllTimers()`, the update would be
synchronously applied. After this change, that will no longer work
because the rendering task (a timer, in this case) isn't scheduled until
after the microtask queue has run.

I don't expect this to be an issue in practice because most people do
not write their tests this way. They either use `act`, or they write
e2e-style tests.

The biggest exception has been... our own internal test suite. Until
recently, many of our tests were written in a way that accidentally
relied on the updates being scheduled synchronously. Over the past few
weeks, @tyao1 and I have gradually converted the test suite to use a new
set of testing helpers that are resilient to this implementation detail.

(There are also some old Relay tests that were written in the style of
React's internal test suite. Those will need to be fixed, too.)

The larger motivation behind this change, aside from a minor performance
improvement, is we intend to use this new microtask to perform
additional logic that doesn't yet exist. Like inferring the priority
of a custom event.
    @acdlite
    acdlite committed Mar 31, 2023
    Configuration menu
    Copy the full SHA
    675b576 View commit details
    Browse the repository at this point in the history

  3. Add flag to disable behavior change

    @acdlite
    acdlite committed Mar 31, 2023
    Configuration menu
    Copy the full SHA
    5c1cf56 View commit details
    Browse the repository at this point in the history