[RFC] Mock react scheduler module #12684
Conversation
**what is the change?:** We want to support calling ReactScheduler multiple times with different callbacks, even if the initial callback hasn't been called yet. There are two possible ways ReactScheduler can handle multiple callbacks, and in one case we know that callbackA depends on callbackB having been called already. For example; callbackA -> updates SelectionState in a textArea callbackB -> processes the deletion of the text currently selected. We want to ensure that callbackA happens before callbackB. For now we will flush callbackB as soon as callbackA is added. In the next commit we'll split this into two methods, which support two different behaviors here. We will support the usual behavior, which would defer both callbackA and callbackB. One issue with this is that we now create a new object to pass to the callback for every use of the scheduler, while before we reused the same object and mutated the 'didExpire' before passing it to each new callback. With multiple callbacks, I think this leads to a risk of mutating the object which is being used by multiple callbacks. **why make this change?:** We want to use this scheduling helper to coordinate between React and non-React scripts. **test plan:** Added and ran a unit test.
**what is the change?:** - Rewires React renderer tests to mock out the react-scheduler **why make this change?:** Once we properly treat this module as external, then we can mock it out and also still run tests against the production bundle. It makes sense to mock this module out, since it's depending heavily on browser timing APIs which are mocked in the jest test environment anyway. We will have separate unit tests covering 'react-scheduler' itself. **test plan:** Run existing tests. **issue:** Internal task T28128480
**what is the change?:** see title **why make this change?:** The ReactScheduler is a separate module from other React modules, and we intend to mock it out in tests. For that mocking to work in tests which run against the bundles, we have to treat ReactScheduler as external. It also just makes sense. **test plan:** Run tests with bundles; yarn test-build
**what is the change?:** NOTE: The last piece to fix is the 'brunch' build. --- Unbreak the 'packaging' fixture to work with the new configuration of 'react-scheduler' as external. **why make this change?:** To allow continued use of this fixture, and also testing how things will work with this new configuration. **test plan:** Ran the build for this fixture and manually inspected
|
ReactDOM: size: -1.0%, gzip: -1.3% Details of bundled changes.Comparing: 9c77ffb...b17cad8 react-dom
react-art
react-scheduler
Generated by 🚫 dangerJS |
|
Fixtures shouldn’t ever need a new |
| @@ -24,6 +24,7 @@ const importSideEffects = Object.freeze({ | |||
| const knownGlobals = Object.freeze({ | |||
| react: 'React', | |||
| 'react-dom': 'ReactDOM', | |||
| 'react-scheduler': 'ReactScheduler', | |||
There was a problem hiding this comment.
I don’t think we want this. We don’t want it to be used in UMD builds if I understand it right.
There was a problem hiding this comment.
Instead we want to add react-scheduler to dependencies of react-dom.
There was a problem hiding this comment.
What does dependencies mean in this context? Things that get inlined into the flat bundle, or things that are left as a require statement (or global reference, for UMD builds)?
The point of the scheduler package is to coordinate React work with non-React work (e.g. Booloader, Webpack, non-React product code) using a centralized scheduler. We want the scheduler to be packaged separately from React and React DOM, not inlined into the flat bundle, so everything can share the same instance.
Since this is a breaking change for UMD bundles, the plan for 16.x is to include the scheduler in the UMD bundle but use the global scheduler if it's available. In 17+, we stop bundling and always use the global scheduler instance.
For CommonJS bundles, it's not a breaking change, so we can externalize it immediately without waiting for 17.
There was a problem hiding this comment.
AFAIK putting it in dependencies should do exactly what you described (for 16).
CJS gets a require, UMD continues bundling (if you remove it from knownGlobals).
| @@ -1,5 +1,6 @@ | |||
| <html> | |||
| <body> | |||
| <script src="../../../build/dist/react-scheduler.development.js"></script> | |||
There was a problem hiding this comment.
Changes adding script tags should not be needed.
|
Thanks - the comments are helpful, will open a separate PR which adds |
This isn't 100% ready but leaving in case @gaearon has time to leave some tips on how this looks so far. I'm still working towards grokking our build system.
This is built on top of another PR, so ignore changes to ReactScheduler itself, which come from #12682