[schedule] Refactor Schedule, remove React-isms #13582

acdlite · 2018-09-06T22:35:12Z

Once the API stabilizes, we will move Schedule into a separate repo. To promote adoption, especially by projects outside the React ecosystem, we'll remove all React-isms from the source and keep it as simple as possible:

No build step.
No static types.
Everything is in a single file.

If we end up needing to support multiple targets, like CommonJS and ESM, we can still avoid a build step by maintaining two copies of the same file, but with different exports.

This commit also refactors the implementation to split out the DOM- specific parts (essentially a requestIdleCallback polyfill). Aside from the architectural benefits, this also makes it possible to write host-agnostic tests. If/when we publish a version of Schedule that targets other environments, like React Native, we can run these same tests across all implementations.

acdlite · 2018-09-06T22:37:13Z

packages/schedule/src/__tests__/ScheduleDOM-test.js

@@ -309,10 +309,10 @@ describe('ScheduleDOM', () => {
          const callbackC = jest.fn(() => callbackLog.push('C'));
          const callbackD = jest.fn(() => callbackLog.push('D'));

-          scheduleWork(callbackA); // won't time out
+          scheduleWork(callbackA, {timeout: 100}); // won't time out


Needed to make a small tweak to an existing test: Callbacks are now executed in timeout-order. This is the only semantic change.

gaearon · 2018-09-06T22:47:38Z

Do you expect to fix the timeout issue I encountered as part of this, or is it completely orthogonal?

pull-bot · 2018-09-06T22:47:38Z

React: size: 🔺+7.2%, gzip: 🔺+5.8%

Details of bundled changes.

Comparing: f6fb03e...3f059be

react

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react.development.js	-1.4%	-1.3%	83.46 KB	82.27 KB	22.63 KB	22.34 KB	UMD_DEV
react.production.min.js	🔺+7.2%	🔺+5.8%	9.54 KB	10.22 KB	3.96 KB	4.18 KB	UMD_PROD

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD
schedule.development.js	-8.5%	-6.7%	15.34 KB	14.04 KB	4.61 KB	4.3 KB	NODE_DEV
schedule.production.min.js	🔺+17.3%	🔺+19.2%	2.78 KB	3.26 KB	1.21 KB	1.45 KB	NODE_PROD
Schedule-dev.js	-8.6%	-7.0%	15.58 KB	14.23 KB	4.66 KB	4.33 KB	FB_WWW_DEV
Schedule-prod.js	🔺+7.9%	🔺+12.7%	7.72 KB	8.33 KB	1.91 KB	2.15 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

acdlite · 2018-09-06T23:01:58Z

@gaearon In a follow-up. Though I might have accidentally fixed it. This PR is just shuffling things around in preparation for bigger changes.

gaearon · 2018-09-06T23:18:39Z

Oh wow. I don't know what exactly changed but the time slicing fixture is now super smooth and doesn't lock up every few seconds. It's actually worrying because I don't like that the experience can change so much without tests also changing.

Interestingly I still don't get into the .didTimeout case you added earlier. But maybe some other change improved scheduling so that we don't actually timeout so often?

acdlite · 2018-09-06T23:29:39Z

Yeah we definitely need more tests. I added some in this PR, planning to write many more.

sebmarkbage · 2018-09-06T23:40:58Z

I think that we should drop all bundling (rollup), all minification, all build scripts for this package. It should just be a single file in its final location. npm publish is literally just the source of the package. It needs to be super clear exactly what you get (the content of a single file) and super unopinionated.

acdlite · 2018-09-06T23:42:16Z

Ok I'll move everything back into one file and just put a big line in between the two halves, like we do with ReactFiberScheduler :D

sebmarkbage · 2018-09-06T23:45:00Z

We should probably drop types and put them in separate libdefs. If we need ES module exports maybe we just have two copies of the file and keep them in sync.

plievone · 2018-09-08T08:06:52Z

I read the tests in this and the continuations pull request briefly through. It seems that most of the tests consume the frame budget exactly, which is nice from the edge case standpoint, but the most common case might be where the last task goes a bit over the frame budget, and it would be good to document this and its variations and implications in the tests.

TrySound · 2018-09-08T08:47:03Z

What's the point of not having a build step? Simple babel+commonjs plugin in prepare hook may work fine without effort in supporting two versions.

gaearon · 2018-09-09T16:50:37Z

Hmm. Not sure what's going on here, but if you put

<html>
  <body>
    <script src="../../../build/dist/react.development.js"></script>
    <script src="../../../build/dist/react-dom.development.js"></script>
    <script src="https://unpkg.com/babel-standalone@6/babel.js"></script>
    <div id="container"></div>
    <script type="text/babel">
      ReactDOM.unstable_createRoot(
        document.getElementById('container')
      ).render(<h1>Hello World!</h1>);
    </script>
  </body>
</html>

into packaging/fixtures/babel-standalone/dev.html, and run yarn build core,dom-client,schedule --type=UMD_DEV, you'll see it not commit anything for five seconds until it expires. Seems like a bug? Surely it can complete a single element faster than five seconds.

gaearon

See the case above — is this a bug?

gaearon · 2018-09-09T17:36:37Z

Specifically it seems like in async mode we don't get deeper than renderRoot until we expire.

gaearon · 2018-09-09T17:48:00Z

packages/schedule/src/Schedule.js

+let isHostCallbackScheduled = false;
+
+let timeRemaining;
+if (hasNativePerformanceNow) {


This is used before declaration. Hence the bugs.

fixed

gaearon · 2018-09-11T19:46:00Z

Can also confirm time slicing fixture still works as intended (incl. the fix to its behavior on timeout) 👍

Once the API stabilizes, we will move Schedule this into a separate repo. To promote adoption, especially by projects outside the React ecosystem, we'll remove all React-isms from the source and keep it as simple as possible: - No build step. - No static types. - Everything is in a single file. If we end up needing to support multiple targets, like CommonJS and ESM, we can still avoid a build step by maintaining two copies of the same file, but with different exports. This commit also refactors the implementation to split out the DOM- specific parts (essentially a requestIdleCallback polyfill). Aside from the architectural benefits, this also makes it possible to write host- agnostic tests. If/when we publish a version of Schedule that targets other environments, like React Native, we can run these same tests across all implementations.

sebmarkbage · 2018-09-12T21:47:47Z

packages/schedule/src/Schedule.js

-    const timeoutId = timeoutIds.get(callback);
-    timeoutIds.delete(callbackId);
-    localClearTimeout(timeoutId);
+  cancelCallback = () => {


Why arrow functions? I figured this would just become an ES file eventually?

Erm I don't get it. Why does that matter?

I mean ES5 file so that it can be published to npm directly.

Ahh, when you use 'ES file', seems we all think it refer to 'ES6+ file'

Should I get rid of let/const as well then?

Yes, let's make it as plain as possible.

gaearon · 2018-09-14T18:41:10Z

packages/schedule/src/Schedule.js

+          flushCallback(firstCallbackNode);
+        } while (
+          firstCallbackNode !== null &&
+          firstCallbackNode.timesOutAt <= getCurrentTime()


Since performance.now() calls have a cost, I wonder if we could avoid recalculating it while we have expired work (since if 100 things expired checking the time is unnecessary — they can't "unexpire"). For example by having a nested loop. You read the time in the outer loop, and compare to it in the inner loop. Once you exit the inner loop, you read the time again. If there's nothing new that expired then we exit.

gaearon · 2018-09-14T18:42:06Z

packages/schedule/src/Schedule.js

+        // Keep flushing callbacks until we run out of time in the frame.
+        while (
+          firstCallbackNode !== null &&
+          getTimeRemaining() - getCurrentTime() > 0


I wonder if this could leave a small buffer as a heuristic. So we don't go over by a few ms from last iteration every time.

gaearon · 2018-09-14T18:43:20Z

packages/schedule/src/Schedule.js

+    let node = firstCallbackNode;
+    do {
+      if (node.timesOutAt > timesOutAt) {
+        // This callback is lower priority than the new one.


I'd like to avoid using the word "priority" in this file if we can since mixing that with timeouts changes the meaning of "higher" and "lower".

e.g.

// The new callback times out before this one

That would also map nicely to the sorted order

gaearon · 2018-09-14T18:44:15Z

packages/schedule/src/Schedule.js

+    } while (node !== firstCallbackNode);
+
+    if (next === null) {
+      // No lower priority callback was found, which means the new callback is


Same re:wording

gaearon · 2018-09-14T18:45:07Z

packages/schedule/src/Schedule.js

  };
+  getTimeRemaining = () => 0;
+} else if (window._sched) {


Maybe window.__scheduleMock or something more obscure?

NE-SmallTown · 2018-09-29T07:12:04Z

packages/schedule/src/Schedule.js

+    isPerformingWork = false;
+    if (firstCallbackNode !== null) {
+      // There's still work remaining. Request another callback.
+      ensureHostCallbackIsScheduled(firstCallbackNode);


@acdlite Seems ensureHostCallbackIsScheduled doesn't use any argument in the function body, can I ask that why we pass the firstCallbackNode here?

For Flow I think

hmm wait this doesn't make sense. I dunno.

Should I remove this？

I think in the initial commit (7c4ff13) ensureHostCallbackIsScheduled expected a param, but the second commit (5f74f04) changed it to just read from the package-level firstCallbackNode variable since that's always the value that was passed in anyway. Seems like the call sites were just overlooked as part of this change.

I think the lint should warn on this but don't know why not

NE-SmallTown · 2018-09-29T07:22:11Z

packages/schedule/src/Schedule.js

+  // to a naive implementation.
+  var timeoutID = -1;
+  requestCallback = function(callback, absoluteTimeout) {
+    timeoutID = setTimeout(callback, 0, true);


@acdlite The absoluteTimeout argument doesn't been used here, I'm not sure why eslint doesn't warn about this. Should I remove it?

* Refactor Schedule, remove React-isms Once the API stabilizes, we will move Schedule this into a separate repo. To promote adoption, especially by projects outside the React ecosystem, we'll remove all React-isms from the source and keep it as simple as possible: - No build step. - No static types. - Everything is in a single file. If we end up needing to support multiple targets, like CommonJS and ESM, we can still avoid a build step by maintaining two copies of the same file, but with different exports. This commit also refactors the implementation to split out the DOM- specific parts (essentially a requestIdleCallback polyfill). Aside from the architectural benefits, this also makes it possible to write host- agnostic tests. If/when we publish a version of Schedule that targets other environments, like React Native, we can run these same tests across all implementations. * Edits in response to Dan's PR feedback

acdlite requested review from bvaughn, sebmarkbage and flarnie September 6, 2018 22:35

facebook-github-bot added the CLA Signed label Sep 6, 2018

acdlite commented Sep 6, 2018

View reviewed changes

acdlite force-pushed the scheduledom branch from 79cae07 to 76d0ecf Compare September 6, 2018 22:59

acdlite force-pushed the scheduledom branch from 76d0ecf to 8d7c627 Compare September 6, 2018 23:32

acdlite force-pushed the scheduledom branch from 8d7c627 to 84f2fe0 Compare September 7, 2018 20:03

acdlite changed the title ~~[schedule] Extract DOM-specific logic into separate module~~ [schedule] Refactor Schedule, remove React-isms Sep 7, 2018

acdlite force-pushed the scheduledom branch from 84f2fe0 to 2c9a100 Compare September 7, 2018 21:06

acdlite mentioned this pull request Sep 7, 2018

[schedule] Nested callbacks and continuations #13595

Closed

gaearon previously requested changes Sep 9, 2018

View reviewed changes

gaearon reviewed Sep 9, 2018

View reviewed changes

gaearon mentioned this pull request Sep 9, 2018

Enable no-use-before-define rule #13606

Merged

acdlite force-pushed the scheduledom branch from 2c9a100 to 3f059be Compare September 11, 2018 18:26

acdlite requested a review from gaearon September 11, 2018 18:26

acdlite force-pushed the scheduledom branch from 3f059be to 7c4ff13 Compare September 12, 2018 21:32

sebmarkbage reviewed Sep 12, 2018

View reviewed changes

Edits in response to Dan's PR feedback

5f74f04

gaearon approved these changes Sep 14, 2018

View reviewed changes

acdlite force-pushed the scheduledom branch from 97ccdc9 to 5f74f04 Compare September 14, 2018 20:54

acdlite merged commit 9f819a5 into facebook:master Sep 14, 2018

alexeyraspopov mentioned this pull request Sep 15, 2018

Remove Flow typings from Schedule.js #13662

Merged

aweary mentioned this pull request Sep 27, 2018

Refactor Scheduler.js #13747

Closed

NE-SmallTown reviewed Sep 29, 2018

View reviewed changes

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

rickhanlonii mentioned this pull request Jan 26, 2021

reasons for using var ？ #20648

Closed

[schedule] Refactor Schedule, remove React-isms #13582

[schedule] Refactor Schedule, remove React-isms #13582

Conversation

acdlite commented Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

gaearon commented Sep 6, 2018

pull-bot commented Sep 6, 2018 • edited Loading

react

schedule

acdlite commented Sep 6, 2018

gaearon commented Sep 6, 2018

acdlite commented Sep 6, 2018

sebmarkbage commented Sep 6, 2018

acdlite commented Sep 6, 2018

sebmarkbage commented Sep 6, 2018

plievone commented Sep 8, 2018

TrySound commented Sep 8, 2018

gaearon commented Sep 9, 2018 • edited Loading

gaearon left a comment

Choose a reason for hiding this comment

gaearon commented Sep 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NE-SmallTown Sep 29, 2018 • edited Loading

Choose a reason for hiding this comment

acdlite commented Sep 6, 2018 •

edited

Loading

pull-bot commented Sep 6, 2018 •

edited

Loading

gaearon commented Sep 9, 2018 •

edited

Loading

acdlite Sep 13, 2018 •

edited

Loading

gaearon Sep 14, 2018 •

edited

Loading

NE-SmallTown Sep 29, 2018 •

edited

Loading