Remove SchedulerHostConfigs #20025
Remove SchedulerHostConfigs #20025
Conversation
|
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 1527bb9:
|
packages/scheduler/index.js
Outdated
| @@ -7,4 +7,37 @@ | |||
|
|
|||
| 'use strict'; | |||
|
|
|||
| export * from './src/Scheduler'; | |||
| const isBrowserEnvironment = | |||
| typeof window !== 'undefined' && typeof MessageChannel === 'function'; | |||
There was a problem hiding this comment.
Could I just drop this check and only export the DOM version if I update the callers during the sync?
There was a problem hiding this comment.
The inline require breaks the build but I'll wait to hear back on this before fixing it.
There was a problem hiding this comment.
For www you can lift the checks into the outer module wrapper, but for OSS this seems like the right place to put them.
There was a problem hiding this comment.
Actually I guess the right place is here? https://github.com/facebook/react/blob/master/packages/scheduler/npm/index.js
I don't know what the best practice is in the npm ecosystem. Ideally we would handle this at the bundler level, using something like the browser field.
| @@ -28,6 +28,7 @@ | |||
| "tracing.js", | |||
| "tracing-profiling.js", | |||
| "unstable_mock.js", | |||
| "unstable_no_dom.js", | |||
There was a problem hiding this comment.
I considered calling the NoDOM version Native since the primary user will be React Native, what do you think about the naming @acdlite?
There was a problem hiding this comment.
To me the primary purpose is to act as a last resort if you accidentally call this in a Node.js environment, like if you run a test without wrapping in act.
React Native will eventually need its own scheduler, so I'd rather not call this one a React Native scheduler since it's not optimized for that at all.
facebook-github-bot
added
CLA Signed
React Core Team
Details of bundled changes.Comparing: 7e405d4...1527bb9 scheduler
react
React: size: -3.8%, gzip: -3.0% Size changes (stable) |
Details of bundled changes.Comparing: 7e405d4...1527bb9 scheduler
react
React: size: -3.5%, gzip: -2.7% Size changes (experimental) |
|
@acdlite There's one test left that's failing and it's related to requiring |
|
@rickhanlonii The test is failing when running against the source files because the checks that you added in It does pass during the So I would either modify the Jest config to go through the |
|
Good point @acdlite, I added a This is ready for review now! |
| @@ -1,6 +1,8 @@ | |||
| 'use strict'; | |||
|
|
|||
| if (process.env.NODE_ENV === 'production') { | |||
| if (typeof window === 'undefined' || typeof MessageChannel !== 'function') { | |||
| module.exports = require('./unstable_no_dom'); | |||
There was a problem hiding this comment.
I'm not sure about this pattern because it's the first time we start doing this kind of switching in the npm wrappers and it won't port nicely to ESM. The thing that we do for production is a static switch which we can move into a compiler but we can't do the same for this.
We should move this branching into the implementation which forces us to treat this nicely in an ESM world.
Summary: Base sync before adding Flight files. This sync includes the following changes: - **[454c2211c](facebook/react@454c2211c )**: Refactor SchedulerHostConfigs ([#20025](facebook/react#20025)) //<Ricky>// - **[56e9feead](facebook/react@56e9feead )**: Remove Blocks ([#20138](facebook/react#20138)) //<Sebastian Markbåge>// - **[3fbd47b86](facebook/react@3fbd47b86 )**: Serialize pending server components by reference (lazy component) ([#20137](facebook/react#20137)) //<Sebastian Markbåge>// - **[930ce7c15](facebook/react@930ce7c15 )**: Allow values to be encoded by "reference" to a value rather than the value itself ([#20136](facebook/react#20136)) //<Sebastian Markbåge>// - **[39eb6d176](facebook/react@39eb6d176 )**: Rename ([#20134](facebook/react#20134)) //<Sebastian Markbåge>// - **[ffd842335](facebook/react@ffd842335 )**: [Flight] Add support for Module References in transport protocol ([#20121](facebook/react#20121)) //<Sebastian Markbåge>// - **[343d7a4a7](facebook/react@343d7a4a7 )**: Fast Refresh: Don't block DevTools commit hook ([#20129](facebook/react#20129)) //<Brian Vaughn>// - **[779a472b0](facebook/react@779a472b0 )**: Prevent inlining into recursive commit functions ([#20105](facebook/react#20105)) //<Andrew Clark>// - **[25b18d31c](facebook/react@25b18d31c )**: Traverse commit phase effects iteratively ([#20094](facebook/react#20094)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions 4e5d7fa...454c221 Reviewed By: rickhanlonii Differential Revision: D24698701 fbshipit-source-id: dfaf692b1051150355dece1657764a484b7ae603
| } | ||
| // There's no pending input. Only yield if we've reached the max | ||
| // yield interval. | ||
| return currentTime >= maxYieldInterval; |
There was a problem hiding this comment.
Should this be currentTime - deadline >= maxYieldInterval?
* Remove SchedulerHostConfigs * Fix builds * Fix forks * Move SchedulerNoDom check to npm/index.js * Fix tests * Add @GATE source * Gate build-only test to build test runs
Summary
Refactors the scheduler to remove SchedulerHostConfigs in favor of separate Schedulers:
Scheduler.js+ DOM branch ofSchedulerHostConfig.default.jsScheduler.js+ fallback branch ofSchedulerHostConfig.default.jsScheduler.js+SchedulerHostConfig.mock.jsFor the mean time, I've included a switch in
index.jsto select either the DOM or NoDOM version.Clients should be updated to select the correct version: