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

Import Scheduler directly, not via host config #14984

Merged
merged 2 commits into from
Mar 6, 2019

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Mar 1, 2019

We currently schedule asynchronous tasks via the host config. (The host config is a static/build-time dependency injection system that varies across different renderers — DOM, native, test, and so on.) Instead of calling platform APIs like requestIdleCallback directly, each renderer implements a method called scheduleDeferredCallback.

We've since discovered that when scheduling tasks, it's crucial that React work is placed in the same queue as other, non-React work on the main thread. Otherwise, you easily end up in a starvation scenario where rendering is constantly interrupted by less important tasks. You need a centralized coordinator that is used both by React and by other frameworks and application code. This coordinator must also have a consistent API across all the different host environments, for convention's sake and so product code is portable — e.g. so the same component can work in both React Native and React Native Web.

This turned into the Scheduler package. We will have different builds of Scheduler for each of our target platforms. With this approach, we treat Scheduler like a built-in platform primitive that exists wherever React is supported.

Now that we have this consistent interface, the indirection of the host config no longer makes sense for the purpose of scheduling tasks. In fact, we explicitly do not want renderers to schedule tasks via any system except the Scheduler package.

So, this PR removes scheduleDeferredCallback and its associated methods from the host config in favor of directly importing Scheduler.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This general change seems fine. CI is failing though.

@sebmarkbage
Copy link
Collaborator

The key here is that we expect isomorphic React code to directly schedule work with the scheduler package and not go through the React package to do so.

Even though you technically could build a renderer with a different host config injection, it would no longer accept idiomatic React code since idiomatic React code would depend on “scheduler”.

acdlite added 2 commits March 6, 2019 14:02
We currently schedule asynchronous tasks via the host config. (The host
config is a static/build-time dependency injection system that varies
across different renderers — DOM, native, test, and so on.) Instead of
calling platform APIs like `requestIdleCallback` directly, each renderer
implements a method called `scheduleDeferredCallback`.

We've since discovered that when scheduling tasks, it's crucial that
React work is placed in the same queue as other, non-React work on the
main thread. Otherwise, you easily end up in a starvation scenario where
rendering is constantly interrupted by less important tasks. You need a
centralized coordinator that is used both by React and by other
frameworks and application code. This coordinator must also have a
consistent API across all the different host environments, for
convention's sake and so product code is portable — e.g. so the same
component can work in both React Native and React Native Web.

This turned into the Scheduler package. We will have different builds of
Scheduler for each of our target platforms. With this approach, we treat
Scheduler like a built-in platform primitive that exists wherever React
is supported.

Now that we have this consistent interface, the indirection of the host
config no longer makes sense for the purpose of scheduling tasks. In
fact, we explicitly do not want renderers to scheduled task via any
system except the Scheduler package.

So, this PR removes `scheduleDeferredCallback` and its associated
methods from the host config in favor of directly importing Scheduler.
@acdlite acdlite force-pushed the importschedulerdirectly branch from a09b8c3 to a74bfb2 Compare March 6, 2019 22:06
@sizebot
Copy link

sizebot commented Mar 6, 2019

Details of bundled changes.

Comparing: 5d49daf...a74bfb2

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js -0.2% -0.4% 618.59 KB 617.14 KB 132.32 KB 131.86 KB RN_FB_DEV
ReactNativeRenderer-prod.js -0.3% -0.4% 246.25 KB 245.58 KB 43.12 KB 42.95 KB RN_FB_PROD
ReactNativeRenderer-profiling.js -0.2% -0.3% 252.43 KB 251.82 KB 44.49 KB 44.34 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js -0.2% -0.3% 618.5 KB 617.05 KB 132.29 KB 131.82 KB RN_OSS_DEV
ReactNativeRenderer-prod.js -0.3% -0.4% 246.26 KB 245.6 KB 43.12 KB 42.95 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js -0.2% -0.3% 252.45 KB 251.84 KB 44.48 KB 44.34 KB RN_OSS_PROFILING
ReactFabric-dev.js -0.2% -0.4% 609.44 KB 607.99 KB 130.04 KB 129.57 KB RN_FB_DEV
ReactFabric-prod.js -0.2% -0.4% 239.32 KB 238.78 KB 41.68 KB 41.52 KB RN_FB_PROD
ReactFabric-profiling.js -0.2% -0.3% 244.66 KB 244.16 KB 43 KB 42.88 KB RN_FB_PROFILING
ReactFabric-dev.js -0.2% -0.4% 609.35 KB 607.9 KB 129.99 KB 129.53 KB RN_OSS_DEV
ReactFabric-prod.js -0.2% -0.4% 239.33 KB 238.78 KB 41.67 KB 41.52 KB RN_OSS_PROD
ReactFabric-profiling.js -0.2% -0.3% 244.67 KB 244.17 KB 42.99 KB 42.87 KB RN_OSS_PROFILING

react-noop-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-noop-renderer.development.js -1.2% -0.8% 24.63 KB 24.33 KB 6.03 KB 5.98 KB NODE_DEV
react-noop-renderer.production.min.js -2.5% -1.4% 9.07 KB 8.84 KB 3.02 KB 2.98 KB NODE_PROD
react-noop-renderer-persistent.development.js -1.2% -0.8% 24.74 KB 24.44 KB 6.04 KB 5.99 KB NODE_DEV
react-noop-renderer-persistent.production.min.js -2.5% -1.4% 9.09 KB 8.86 KB 3.02 KB 2.98 KB NODE_PROD
react-noop-renderer-server.development.js 0.0% +0.1% 1.83 KB 1.83 KB 875 B 876 B NODE_DEV
react-noop-renderer-server.production.min.js 0.0% 🔺+0.2% 813 B 813 B 489 B 490 B NODE_PROD

Generated by 🚫 dangerJS

@acdlite acdlite merged commit 1e3b619 into facebook:master Mar 6, 2019
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
* Import Scheduler directly, not via host config

We currently schedule asynchronous tasks via the host config. (The host
config is a static/build-time dependency injection system that varies
across different renderers — DOM, native, test, and so on.) Instead of
calling platform APIs like `requestIdleCallback` directly, each renderer
implements a method called `scheduleDeferredCallback`.

We've since discovered that when scheduling tasks, it's crucial that
React work is placed in the same queue as other, non-React work on the
main thread. Otherwise, you easily end up in a starvation scenario where
rendering is constantly interrupted by less important tasks. You need a
centralized coordinator that is used both by React and by other
frameworks and application code. This coordinator must also have a
consistent API across all the different host environments, for
convention's sake and so product code is portable — e.g. so the same
component can work in both React Native and React Native Web.

This turned into the Scheduler package. We will have different builds of
Scheduler for each of our target platforms. With this approach, we treat
Scheduler like a built-in platform primitive that exists wherever React
is supported.

Now that we have this consistent interface, the indirection of the host
config no longer makes sense for the purpose of scheduling tasks. In
fact, we explicitly do not want renderers to scheduled task via any
system except the Scheduler package.

So, this PR removes `scheduleDeferredCallback` and its associated
methods from the host config in favor of directly importing Scheduler.

* Missed an extraneous export
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants