Skip to content

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 5, 2018

Fixes #12980 (comment).

We can't access a global if it's not defined so we should always guard it. I consolidated these reads in one place. I also changed the test to reproduce the problem on master.

const localCancelAnimationFrame =
typeof cancelAnimationFrame === 'function'
? cancelAnimationFrame
: (undefined: any);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This module throws if this is undefined, right? Maybe we should use an invariant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't want to spend the bytes.

I think it actually used an invariant in the past, and Sebastian wanted to relax that to a warning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess in sync mode this module is never used so an invariant would be harsh

@pull-bot
Copy link

pull-bot commented Jul 5, 2018

ReactDOM: size: 🔺+0.1%, gzip: 0.0%

Details of bundled changes.

Comparing: 88d7ed8...9064cf5

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% 0.0% 638.02 KB 638.46 KB 149.34 KB 149.41 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 0.0% 95.59 KB 95.69 KB 30.9 KB 30.91 KB UMD_PROD
react-dom.development.js +0.1% 0.0% 634.15 KB 634.58 KB 148.17 KB 148.23 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 0.0% 95.57 KB 95.67 KB 30.41 KB 30.42 KB NODE_PROD
react-dom.profiling.min.js +0.1% 0.0% 96.54 KB 96.64 KB 30.75 KB 30.76 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.1% 422.62 KB 423.06 KB 95.02 KB 95.1 KB UMD_DEV
react-art.production.min.js 🔺+0.1% -0.0% 82.71 KB 82.81 KB 25.47 KB 25.47 KB UMD_PROD
react-art.development.js +0.1% +0.1% 354.6 KB 355.04 KB 77.89 KB 77.96 KB NODE_DEV
react-art.production.min.js 🔺+0.2% 🔺+0.1% 47.69 KB 47.79 KB 14.85 KB 14.86 KB NODE_PROD

react-scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-scheduler.development.js +2.7% +1.5% 16.3 KB 16.74 KB 5.07 KB 5.15 KB UMD_DEV
react-scheduler.production.min.js 🔺+3.6% 🔺+0.9% 2.62 KB 2.72 KB 1.24 KB 1.25 KB UMD_PROD
react-scheduler.development.js +2.7% +1.6% 16.11 KB 16.55 KB 5.03 KB 5.11 KB NODE_DEV
react-scheduler.production.min.js 🔺+3.7% 🔺+0.7% 2.54 KB 2.64 KB 1.19 KB 1.2 KB NODE_PROD
ReactScheduler-dev.js +3.3% +1.6% 14.87 KB 15.36 KB 4.55 KB 4.63 KB FB_WWW_DEV
ReactScheduler-prod.js 🔺+1.6% 🔺+0.3% 7.47 KB 7.59 KB 1.9 KB 1.91 KB FB_WWW_PROD

Generated by 🚫 dangerJS

Copy link
Contributor

@flarnie flarnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, thanks for fixing. Also thinking we may want to throw if the scheduler starts to try and run and rAF is still undefined. Will consider that for the future.

@gaearon gaearon merged commit 1386ccd into facebook:master Jul 5, 2018
@gaearon gaearon deleted the fix-raf branch July 5, 2018 18:42
@bvaughn
Copy link
Contributor

bvaughn commented Jul 6, 2018

Nice! thanks for the fix :)

Edit Confirmed that this fixes the regression in the open source project I first noticed it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants