Remove rAF fork #12980

flarnie · 2018-06-04T18:57:21Z

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:

Land this PR on Github.
Flarnie will quickly run a sync getting this change into www.
We delete the internal forked version of
'requestAnimationFrameForReact'.
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

pull-bot · 2018-06-04T19:06:33Z

ReactDOM: size: -0.1%, gzip: -0.1%

Details of bundled changes.

Comparing: 1594409...e10ad4c

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	-0.1%	-0.0%	626.67 KB	626.26 KB	145.85 KB	145.83 KB	UMD_DEV
react-dom.production.min.js	-0.1%	-0.1%	94.25 KB	94.15 KB	30.53 KB	30.5 KB	UMD_PROD
react-dom.development.js	-0.1%	-0.0%	611.04 KB	610.62 KB	141.86 KB	141.83 KB	NODE_DEV
react-dom.production.min.js	-0.1%	-0.1%	92.74 KB	92.65 KB	29.52 KB	29.49 KB	NODE_PROD
react-dom.profiling.min.js	-0.1%	-0.1%	93.65 KB	93.55 KB	29.86 KB	29.82 KB	NODE_PROFILING

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	-0.1%	-0.0%	408.25 KB	407.84 KB	91.1 KB	91.08 KB	UMD_DEV
react-art.production.min.js	-0.1%	-0.1%	81.12 KB	81.02 KB	25.01 KB	24.98 KB	UMD_PROD
react-art.development.js	-0.1%	-0.0%	334.11 KB	333.69 KB	72.19 KB	72.18 KB	NODE_DEV
react-art.production.min.js	-0.2%	-0.2%	45.48 KB	45.38 KB	14.16 KB	14.14 KB	NODE_PROD

react-scheduler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-scheduler.development.js	-10.1%	-7.7%	19.17 KB	17.22 KB	5.74 KB	5.3 KB	UMD_DEV
react-scheduler.production.min.js	-23.0%	-22.5%	3.16 KB	2.43 KB	1.53 KB	1.18 KB	UMD_PROD
react-scheduler.development.js	-4.1%	-1.9%	14.11 KB	13.54 KB	4.25 KB	4.17 KB	NODE_DEV
react-scheduler.production.min.js	-16.2%	-16.6%	2.76 KB	2.31 KB	1.35 KB	1.13 KB	NODE_PROD
ReactScheduler-dev.js	-3.0%	-1.4%	14.11 KB	13.69 KB	4.26 KB	4.2 KB	FB_WWW_DEV
ReactScheduler-prod.js	-9.9%	-13.4%	7.62 KB	6.86 KB	2.06 KB	1.78 KB	FB_WWW_PROD

Generated by 🚫 dangerJS

gaearon · 2018-06-05T19:46:49Z

packages/react-scheduler/src/ReactScheduler.js

+  ) {
+    warning(
+      false,
+      'schedule depends on requestAnimationFrame. Make sure that you load a ' +


"schedule depends on requestAnimationFrame" doesn't make it obvious "schedule" is a name so it reads a bit odd. Maybe we can reword the whole message to not mention the library name? e.g.

This browser doesn't support requestAnimationFrame. Make sure [...]

Absolutely - thanks!

gaearon

Looks OK but see the nit

gaearon · 2018-06-05T19:47:49Z

(Will also need a Jest test fix)

flarnie · 2018-06-07T18:25:22Z

Thanks for the review!
Have been pretty tied up with travel, but aiming to polish and test this and try syncing it in early next week.

**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 · 2018-06-12T21:55:40Z

I've manually tested this and it seems ok - going to try and land and quickly sync early tomorrow morning.

flarnie · 2018-06-12T23:11:31Z

Found a potential issue internally, will debug that before landing this on Github. See internal task T29442940

bvaughn · 2018-07-05T04:04:01Z

I think this changed causes a runtime error in the jsdom environment which doesn't occur in the latest release (16.4.1). I discovered this while testing the DevTools profiler on sites shared by open source users. One of those sites uses jsdom as part of the production build process.

flarnie · 2018-07-05T17:09:50Z

@bvaughn If you remember which site it was, message me a link?
This should only affect the Facebook build, and possibly test runs that are using async mode.

gaearon · 2018-07-05T17:23:19Z

Why does it only affect the Facebook build? Changes in the scheduler code would affect all builds.

Specifically, I think the issue might be that we warn when typeof requestAnimationFrame !== 'function', but then we read it anyway (which would cause a reference error in strict mode if it's really not defined).

gaearon · 2018-07-05T17:34:58Z

#13152

flarnie · 2018-07-05T18:17:30Z

Thanks for the quick fix! I see that we always should guard when accessing a possibly not defined global API.

I know we wanted to avoid using invariant to avoid sending the bytes, reduce dependency, but I also wonder if it makes sense to throw an error if we reach that part of the code and requestAnimationFrame is not defined? Or maybe just at the point where we try to use it?

Why does it only affect the Facebook build? Changes in the scheduler code would affect all builds.

The scheduler should only be used in async mode, and the only change there was a DEV only warning, hence my thinking that only tests running in async would fail, no production behavior would be expected to change. I see now that even requiring this module without a rAF polyfill could lead to an error, without the guarding you're adding.

flarnie requested review from acdlite and gaearon June 4, 2018 18:57

facebook-github-bot added the CLA Signed label Jun 4, 2018

audiolion mentioned this pull request Jun 5, 2018

Revisit the decision to allow webpack analyzer tools facebook/create-react-app#4563

Closed

gaearon reviewed Jun 5, 2018

View reviewed changes

gaearon approved these changes Jun 5, 2018

View reviewed changes

flarnie added 3 commits June 11, 2018 16:31

fix nits

3db125f

fix tests, fix changes from rebasing

c93491b

flarnie force-pushed the removeForkOfRaf branch from 6256737 to c93491b Compare June 12, 2018 17:29

fix lint

e10ad4c

flarnie merged commit 2a80859 into facebook:master Jun 13, 2018

gaearon mentioned this pull request Jul 5, 2018

Fix ReferenceError when requestAnimationFrame isn't defined #13152

Merged

Remove rAF fork #12980

Remove rAF fork #12980

Uh oh!

Conversation

flarnie commented Jun 4, 2018

Uh oh!

pull-bot commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

react-dom

react-art

react-scheduler

Uh oh!

gaearon Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

flarnie Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

gaearon commented Jun 5, 2018

Uh oh!

flarnie commented Jun 7, 2018

Uh oh!

flarnie commented Jun 12, 2018

Uh oh!

flarnie commented Jun 12, 2018

Uh oh!

bvaughn commented Jul 5, 2018

Uh oh!

flarnie commented Jul 5, 2018

Uh oh!

gaearon commented Jul 5, 2018

Uh oh!

gaearon commented Jul 5, 2018

Uh oh!

flarnie commented Jul 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pull-bot commented Jun 4, 2018 •

edited

Loading