Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

rickhanlonii · 2021-01-26T03:23:45Z

Overview

We're researching a few updates to discrete events:

For concurrent mode, we want to revert back to flushing discrete events synchronously so that the upgrade path is easier.
To help catch bugs and make the behavior more consistent, we're going to schedule the sync flush in a microtask. This ensures that the update is "async", but is still flushed synchronously at the end of the current task, in order, before other work.
To schedule the microtask, this diff adds scheduleMicrotaskCallback to the scheduler along with a basic queueMicrotask pollyfill.

Note: there are a few tests that changes and others that are still disabled. Hope to work those use cases out in review.

acdlite · 2021-01-26T03:26:47Z

Do you mind splitting the queueMicrotask stuff into its own PR? Then stack the rest on top

packages/react-dom/src/__tests__/ReactDOMHooks-test.js

sizebot · 2021-01-26T03:30:32Z

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against cf96b27

sizebot · 2021-01-26T03:30:36Z

	Warnings
⚠️	Build job for base commit is still pending: `a6b5256`

Size changes (stable)

Generated by 🚫 dangerJS against cf96b27

sebmarkbage · 2021-01-26T03:32:08Z

I wasn't thinking that queueMicrotask would be integrated into the scheduler since it's not natively neither. They're separate things.

Is there a reason that this isn't literally export scheduleMicrotask = typeof queueMicrotask === 'function' ? queueMicrotask : ... when it's available? That's the minimal overhead.

packages/scheduler/src/queueMicrotask.js

rickhanlonii · 2021-01-26T03:30:13Z

packages/react-reconciler/src/SchedulerWithReactIntegration.new.js

@@ -27,6 +27,7 @@ import {
 const {
  unstable_runWithPriority: Scheduler_runWithPriority,
  unstable_scheduleCallback: Scheduler_scheduleCallback,
+  unstable_scheduleMicrotaskCallback: Scheduler_scheduleMicrotaskCallback,


I don't have a strong opinion on the API.

An alternative may be:

scheduleCallback( MicrotaskPriority, performSyncWorkOnRoot.bind(null, root), );

I think it should just be queueMicrotask. It's just a literal polyfill for that function which is already a standard. I'm a little doubtful that it'll be part of the Scheduler spec eventually since microtasks are layered differently spec wise, but even if it was that API has more overhead than a simple call into native.

rickhanlonii · 2021-01-26T03:31:55Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

@@ -232,7 +232,12 @@ describe('ReactIncrementalErrorHandling', () => {
    expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]);
  });

-  it("retries at a lower priority if there's additional pending work", () => {
+  it("retries at a lower priority if there's additional pending work", async () => {
+    if (gate(flags => flags.enableDiscreteEventMicroTasks)) {


@acdlite there are 3 tests disabled in this file, which I'm not sure how to fix without an microtask integration with toFlushAndYieldThrough

rickhanlonii · 2021-01-26T03:33:19Z

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js

-    });
-    // Low pri
-    root.render(<App text="A" shouldSuspend={true} />);
+    if (gate(flags => flags.enableDiscreteEventMicroTasks)) {


These tests are not as good as before since we can't flush only the microtasks without all the other work.

rickhanlonii · 2021-01-26T03:36:23Z

packages/scheduler/src/queueMicrotask.js

+    Promise.resolve(null)
+      .then(callback)
+      .catch(e => {
+        setTimeout(() => {


Important note: when we fallback to the promise implementation, we have to push errors into a new task or they'll be treated as unhandled promise rejections. I'm using the MDN polyfill version here, but we may want to schedule a MicrotaskPriority task instead, since these can be pushed behind other work.

Not too concerned since it really only affects debugging and all modern browsers support queueMicrotask. (God I hate this problem so much, though.)

rickhanlonii · 2021-01-26T03:42:47Z

@acdlite I can break this up, do you agree with @sebmarkbage that this shouldn't be in scheduler? If so, where should this go?

sebmarkbage · 2021-01-26T03:44:22Z

I'm not sure where this should live but the part I don't get is why it's adding it to a scheduler queue? It seems like it should just be straight passthrough.

It could be part of scheduler just to have one convenient way to get all these polyfills but now I'm skeptical it belongs there because it's tempting to integrate it unnecessarily and you have to always pull in the whole scheduler even if you depend on only this function. Nobody else is going to do that so there's no code sharing benefit.

acdlite · 2021-01-26T03:45:56Z

I would do it as an export of Scheduler for now.

import queueMicrotask from 'scheduler/queueMicrotask';

sebmarkbage · 2021-01-26T03:45:59Z

Maybe it should just be a host config?

acdlite · 2021-01-26T03:48:05Z

Arguably all of the Scheduler methods should be host configs so we can possibly swap them out postTask one day without the Scheduler package indirection. (So, the same rationale for not wrapping queueMicrotask). But I don't want to bikeshed this right now so just put it somewhere and we can figure out the host config -> Scheduler boundary later.

acdlite · 2021-01-26T03:50:52Z

If we publish a timer polyfill we'll likely put that in the Scheduler package, too. We also may need to publish our own Promise polyfill for older browsers, so it integrates properly with our message event listener. I don't think we're going to solve all of that in this PR.

sebmarkbage · 2021-01-26T03:50:56Z

I was thinking that for React Native it might be that discrete !== micro task. Since we can implement it differently there and some discrete might need to consider the thread they're on. Seems like Host Config might be the better injection point there but could be layered twice. Like the DOM host config defers to scheduler/queueMicrotask. Where as for scheduling work, it makes sense to have a shared one since it's public API and integrates with other scheduled things.

acdlite · 2021-01-26T03:56:48Z

We could call it like queueDiscrete or something. That's fine, though feels like too early of an abstraction right now and I wouldn't block the PR over it. I would put a TODO and then revisit when React Native is ready.

rickhanlonii · 2021-01-26T03:57:42Z

@sebmarkbage one key benefit of using the scheduler for this right now is that we call cancelCallback when the priority changes. With this implementation, that just works. With a separate queueMicrotask impl, we'd have to sort out that cancelation. I know @acdlite had a suggestion for how to remove that dependency on cancelCallback, but I didn't entirely follow it.

sebmarkbage · 2021-01-26T03:59:13Z

We can't cancel discrete work though. So that won't make sense anyway.

acdlite · 2021-01-26T03:59:30Z

@rickhanlonii The suggestion is to not cancel the microtask once it's scheduled 😬

rickhanlonii · 2021-01-26T04:01:58Z

We do need to cancel non-discrete tasks once the priority changes to discrete (like a child that uses startTransition and a parent that doesn't), so we'll need to only cancel the non-discrete tasks somehow, which seems non-trivial to unblock this.

rickhanlonii · 2021-01-26T04:03:02Z

Maybe it is trivial, and we just don't store the microtask on the node.

acdlite · 2021-01-26T04:07:02Z

Let's continue in the separate PR? That way we can focus on that by itself

codesandbox-ci · 2021-01-26T18:27:35Z

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 cf96b27:

Sandbox	Source
React	Configuration

acdlite · 2021-01-26T18:46:12Z

packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js

@@ -369,19 +373,24 @@ describe('SimpleEventPlugin', function() {
      click();

      // Flush the remaining work
-      Scheduler.unstable_flushAll();
+      await TestUtils.act(async () => {


act should wrap click

rickhanlonii · 2021-01-26T23:42:15Z

Thanks for the feedback.

I went with the HostConfig strategy and created a new stack of PRs with the head here.

rickhanlonii requested review from gaearon and acdlite January 26, 2021 03:23

facebook-github-bot added the CLA Signed label Jan 26, 2021

acdlite reviewed Jan 26, 2021

View reviewed changes

packages/react-dom/src/__tests__/ReactDOMHooks-test.js Show resolved Hide resolved

sebmarkbage reviewed Jan 26, 2021

View reviewed changes

packages/scheduler/src/queueMicrotask.js Outdated Show resolved Hide resolved

rickhanlonii commented Jan 26, 2021

View reviewed changes

rickhanlonii force-pushed the rh-scheduler-updates branch from 41a02ed to 41eeb82 Compare January 26, 2021 18:22

acdlite reviewed Jan 26, 2021

View reviewed changes

Improve tests that use discrete events

cf96b27

rickhanlonii force-pushed the rh-scheduler-updates branch from 41eeb82 to cf96b27 Compare January 26, 2021 23:28

rickhanlonii closed this Jan 26, 2021

rickhanlonii deleted the rh-scheduler-updates branch January 27, 2021 02:41

rickhanlonii added the React Core Team Opened by a member of the React Core Team label Feb 11, 2021

rickhanlonii mentioned this pull request Mar 30, 2023

Move update scheduling to microtask #26512

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

rickhanlonii commented Jan 26, 2021

acdlite commented Jan 26, 2021

sizebot commented Jan 26, 2021 •

edited

Loading

sizebot commented Jan 26, 2021 •

edited

Loading

sebmarkbage commented Jan 26, 2021

rickhanlonii Jan 26, 2021

sebmarkbage Jan 26, 2021 •

edited

Loading

rickhanlonii Jan 26, 2021

rickhanlonii Jan 26, 2021

rickhanlonii Jan 26, 2021

acdlite Jan 26, 2021

rickhanlonii commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021 •

edited

Loading

acdlite commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021

acdlite commented Jan 26, 2021

acdlite commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021 •

edited

Loading

acdlite commented Jan 26, 2021

rickhanlonii commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021

acdlite commented Jan 26, 2021

rickhanlonii commented Jan 26, 2021

rickhanlonii commented Jan 26, 2021

acdlite commented Jan 26, 2021

codesandbox-ci bot commented Jan 26, 2021 •

edited

Loading

acdlite Jan 26, 2021

rickhanlonii commented Jan 26, 2021

Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

Conversation

rickhanlonii commented Jan 26, 2021

Overview

acdlite commented Jan 26, 2021

sizebot commented Jan 26, 2021 • edited Loading

Size changes (experimental)

sizebot commented Jan 26, 2021 • edited Loading

Size changes (stable)

sebmarkbage commented Jan 26, 2021

rickhanlonii Jan 26, 2021

Choose a reason for hiding this comment

sebmarkbage Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

rickhanlonii Jan 26, 2021

Choose a reason for hiding this comment

rickhanlonii Jan 26, 2021

Choose a reason for hiding this comment

rickhanlonii Jan 26, 2021

Choose a reason for hiding this comment

acdlite Jan 26, 2021

Choose a reason for hiding this comment

rickhanlonii commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021 • edited Loading

acdlite commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021

acdlite commented Jan 26, 2021

acdlite commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021 • edited Loading

acdlite commented Jan 26, 2021

rickhanlonii commented Jan 26, 2021

sebmarkbage commented Jan 26, 2021

acdlite commented Jan 26, 2021

rickhanlonii commented Jan 26, 2021

rickhanlonii commented Jan 26, 2021

acdlite commented Jan 26, 2021

codesandbox-ci bot commented Jan 26, 2021 • edited Loading

acdlite Jan 26, 2021

Choose a reason for hiding this comment

rickhanlonii commented Jan 26, 2021

sizebot commented Jan 26, 2021 •

edited

Loading

sizebot commented Jan 26, 2021 •

edited

Loading

sebmarkbage Jan 26, 2021 •

edited

Loading

sebmarkbage commented Jan 26, 2021 •

edited

Loading

sebmarkbage commented Jan 26, 2021 •

edited

Loading

codesandbox-ci bot commented Jan 26, 2021 •

edited

Loading