Temporary fix for grabbing wrong rAF polyfill in ReactScheduler #12837
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**what is the change?:** For now... We need to grab a slightly different implementation of rAF internally at FB than in Open Source. Making rAF a dependency of the ReactScheduler module allows us to fork the dependency at FB. NOTE: After this lands we have an alternative plan to make this module separate from React and require it before our Facebook timer polyfills are applied. But want to land this now to keep master in a working state and fix bugs folks are seeing at Facebook. Thanks @sebmarkbage @acdlite and @sophiebits for discussing the options and trade-offs for solving this issue. **why make this change?:** This fixes a problem we're running into when experimenting with ReactScheduler internally at Facebook, **and* it's part of our long term plan to use dependency injection with the scheduler to make it easier to test and adjust. **test plan:** Ran tests, lint, flow, and will manually test when syncing into Facebook's codebase. **issue:** See internal task T29442940
|
After landing this will do a quick sync and create the shim on the FB side. |
acdlite
approved these changes
May 17, 2018
flarnie
added a commit
to flarnie/react
that referenced
this pull request
Jun 4, 2018
**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
Merged
flarnie
added a commit
to flarnie/react
that referenced
this pull request
Jun 12, 2018
**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
flarnie
added a commit
that referenced
this pull request
Jun 13, 2018
* 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what is the change?:
For now...
We need to grab a slightly different implementation of rAF internally at
FB than in Open Source. Making rAF a dependency of the ReactScheduler
module allows us to fork the dependency at FB.
NOTE: After this lands we have an alternative plan to make this module
separate from React and require it before our Facebook timer polyfills
are applied. But want to land this now to keep master in a working state
and fix bugs folks are seeing at Facebook. This is basically the same as previously closed PR #12831
Thanks @sebmarkbage @acdlite and @sophiebits for discussing the options
and trade-offs for solving this issue.
why make this change?:
This fixes a problem we're running into when experimenting with
ReactScheduler internally at Facebook, *and it's part of our long term
plan to use dependency injection with the scheduler to make it easier to
test and adjust.
test plan:
Ran tests, lint, flow, and will manually test when syncing into
Facebook's codebase.
issue:
See internal task T29442940