[scheduler] 4/n Allow splitting out schedule in fb-www, prepare to fix polyfill issue internally
#12900
Conversation
|
Why do we want it bundled in open source? Seems like the same thing can happen there, and we know that we'll need ART and DOM to probably coordinate. |
It will be useful to have this as a separate package in OS, after we finish implementing error handling, add proper docs, and have tried it with non-React code internally. I should publish it with a similar |
flarnie
force-pushed
the
useReferencesToGlobalsInScheduler
branch
from
48fe169 to
3bc34c5
Compare
flarnie
force-pushed
the
useReferencesToGlobalsInScheduler
branch
3 times, most recently
from
3707b89 to
de54d2e
Compare
|
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2% Details of bundled changes.Comparing: 001f9ef...1320890 react-dom
react-art
react-scheduler
Generated by 🚫 dangerJS |
flarnie
force-pushed
the
useReferencesToGlobalsInScheduler
branch
from
de54d2e to
4ac1a80
Compare
| now, | ||
| scheduleWork, | ||
| cancelScheduledWork, | ||
| } from 'react-scheduler/src/ReactScheduler'; |
There was a problem hiding this comment.
This indirection seems a bit complicated :-(
Do we want to keep this fork long term? Is there a better fix in mind? How will this ultimately work in open source when the package is ready?
There was a problem hiding this comment.
It is complicated, agreed.
Do we want to keep this fork long term? Is there a better fix in mind? How will this ultimately work in open source when the package is ready?
Here is my understanding of how it will work -
- Once the
scheduleAPI is stable, we publish it as a separate module and make it a dependency ofreact-domandreact-art. In OS people just pull in theschedulebundle via npm, and it uses the nativerequestAnimationFrame. - We would still need something like this for the 'www' build of 'react-dom', because we still have the problem with our 'requestAnimationFrame' polyfill in 'www'. Some of the behavior of the polyfill doesn't work, but some of the custom behavior we still need. I think at that point we can get away with the following simpler approach in 'www':
- we still require 'schedule' before the polyfill runs, which means 'schedule' pulls in the native rAF and uses it.
- In 'www' we write a wrapper module that adds the 'timeSlice' wrapper to 'schedule' itself, which we still need, and React would grab the wrapped version. Not sure if we'll still need a fork/shim for that to work.
It took a lot of discussion to agree on this solution, I'm hesitant to go back to the drawing board when we have something that works.
Regarding my original solution of just shimming 'requestAnimationFrame' itself in our 'www' build - @sebmarkbage had concerns that we should not be using the polyfilled version of 'requestAnimationFrame' at all in 'www', and that the best way to avoid pulling in the polyfill was to require the schedule module before the polyfilling happens.
There was a problem hiding this comment.
Actually, if we just wrap the 'callback' before we pass it into 'scheduleWork' at every single callsite, or even if we just expose a wrapped version of 'schedule' for non-React use within 'www', then I think we can still avoid a fork of 'schedule'. After we publish it as a separate module, that is.
| // We capture a local reference to any global, in case it gets polyfilled after | ||
| // this module is initially evaluated. | ||
| // We want to be using a consistent implementation. | ||
| const localDate = Date; |
There was a problem hiding this comment.
Nit: Remove the local prefix
const Date = window.Date;| // We capture a local reference to any global, in case it gets polyfilled after | ||
| // this module is initially evaluated. | ||
| // We want to be using a consistent implementation. | ||
| const localRequestAnimationFrame = requestAnimationFrame; |
There was a problem hiding this comment.
edit: looking again, maybe it's not a Flow error if we do window.Date like you actually suggested. Still feels weird but less weird than const Date = Date;.
Just one thing - which I think is why I did this in the first place. It's a Flow error, and seems like bad practice, to use a variable name which is also a global keyword.
I could do RequestAnimationFrame but that seems easier to be confused about. The local prefix means this is the local variable holding a reference to that API.
**what is the change?:** See title **why make this change?:** We want to avoid initially calling one version of an API and then later accessing a polyfilled version. **test plan:** Run existing tests.
**what is the change?:** In 'www' we want to reference the separate build of ReactScheduler, which allows treating it as a separate module internally. **why make this change?:** We need to require the ReactScheduler before our rAF polyfill activates, in order to customize which custom behaviors we want. This is also a step towards being able to experiment with using it outside of React. **test plan:** Ran tests, ran the build, and ran `test-build`.
**what is the change?:** See title **why make this change?:** Splitting out the 'schedule' module allows us to load it before polyfills kick in for rAF and other APIs. And long term we want to split this into a separate module anyway, this is a step towards that. **test plan:** I'll run the sync next week and verify that this all works. :)
|
There is a test failing locally, not sure why. Investigating now. |
|
Ah - just needed to |
flarnie
force-pushed
the
useReferencesToGlobalsInScheduler
branch
from
4ac1a80 to
1320890
Compare
**what is the change?:** Undid facebook#12837 **why make this change?:** We originally forked rAF because we needed to pull in a particular version of rAF internally at Facebook, to avoid grabbing the default polyfilled version. The longer term solution, until we can get rid of the global polyfill behavior, is to initialize 'schedule' before the polyfilling happens. Now that we have landed and synced facebook#12900 successfully, we can initialize 'schedule' before the polyfill runs. So we can remove the rAF fork. Here is how it will work: 1. Land this PR on Github. 2. Flarnie will quickly run a sync getting this change into www. 3. We delete the internal forked version of 'requestAnimationFrameForReact'. 4. We require 'schedule' in the polyfill file itself, before the polyfilling happens. **test plan:** Flarnie will manually try the above steps locally and verify that things work. **issue:** Internal task T29442940
**what is the change?:** Undid facebook#12837 **why make this change?:** We originally forked rAF because we needed to pull in a particular version of rAF internally at Facebook, to avoid grabbing the default polyfilled version. The longer term solution, until we can get rid of the global polyfill behavior, is to initialize 'schedule' before the polyfilling happens. Now that we have landed and synced facebook#12900 successfully, we can initialize 'schedule' before the polyfill runs. So we can remove the rAF fork. Here is how it will work: 1. Land this PR on Github. 2. Flarnie will quickly run a sync getting this change into www. 3. We delete the internal forked version of 'requestAnimationFrameForReact'. 4. We require 'schedule' in the polyfill file itself, before the polyfilling happens. **test plan:** Flarnie will manually try the above steps locally and verify that things work. **issue:** Internal task T29442940
* Remove rAF fork **what is the change?:** Undid #12837 **why make this change?:** We originally forked rAF because we needed to pull in a particular version of rAF internally at Facebook, to avoid grabbing the default polyfilled version. The longer term solution, until we can get rid of the global polyfill behavior, is to initialize 'schedule' before the polyfilling happens. Now that we have landed and synced #12900 successfully, we can initialize 'schedule' before the polyfill runs. So we can remove the rAF fork. Here is how it will work: 1. Land this PR on Github. 2. Flarnie will quickly run a sync getting this change into www. 3. We delete the internal forked version of 'requestAnimationFrameForReact'. 4. We require 'schedule' in the polyfill file itself, before the polyfilling happens. **test plan:** Flarnie will manually try the above steps locally and verify that things work. **issue:** Internal task T29442940 * fix nits * fix tests, fix changes from rebasing * fix lint
Edit: This PR has been rebased and no longer has an cruft from previous PRs.
what is the change?:
www, planning to use the generated bundle instead of inlining it.react-schedulerbundle forfb-wwwwhy make this change?:
Context: internally we polyfill
requestAnimationFrameandsetTimeoutand other global APIs. These "polyfills" also add and modify the functionality of the APIs, in ways that are not compatible with React.We need more control over the version of those APIs that is used by the
schedulemodule. To manage this, we want to try loading theschedulemodule before the polyfills are loaded, and holding a reference to the originalrequestAnimationFrameAPI.To require
scheduleearly we need to split it out from React. This diff takes a simple approach to that - we still wantschedulebundled with React for open source, but internally we want it to be a separate module.test plan:
I'll run the sync next week and verify that this all works. :)
issue:
See internal task T29442940