Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Do not include package-local polyfills. #1867

Closed
wants to merge 1 commit into from
Closed

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Jun 7, 2017

This removes babel's core-js polyfills from the distributed NPM builds. This aids in further reducing app bundle size when targeting modern browsers, and allows flexibility in how these polyfills are provided if necessary, which can help fix React Native specific issues, as illustrated in #1799 and #1704 and #1818.

The drawback is that polyfills for these things are now something that client developers will need to think about directly. Shipping Relay to older browsers before this diff should work fine, but after this diff will require global polyfills. As such, this diff may constitute at least a minor-breaking change.

@kassens
Copy link
Member

kassens commented Jun 8, 2017

Looks good, thanks for trimming this down 👍

@leebyron leebyron force-pushed the no-polyfills branch 3 times, most recently from cd1807c to b592f28 Compare June 8, 2017 03:15
@facebook-github-bot
Copy link
Contributor

@leebyron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@leebyron leebyron force-pushed the no-polyfills branch 3 times, most recently from 71cc27c to e63ef05 Compare June 8, 2017 04:20
@facebook-github-bot
Copy link
Contributor

@leebyron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@leebyron leebyron force-pushed the no-polyfills branch 3 times, most recently from 0d4a502 to 35a8cbd Compare June 8, 2017 07:43
@leebyron
Copy link
Contributor Author

leebyron commented Jun 8, 2017

Ok, so of course this ended up being way more complicated than originally anticipated.

Using native Promises instead of polyfilled Promises broke a bunch of tests since native Promises are implemented using setTimeout() which can be controlled via jest.runAllTimers() however native Promises use the micro-task queue which jest does not control. This means doing some test refactoring. So I've added to this patch some refactored tests using async/await.

@leebyron leebyron force-pushed the no-polyfills branch 2 times, most recently from 4a01e9c to 82825ee Compare June 8, 2017 08:48
This removes babel's core-js polyfills from the distributed NPM builds. This aids in further reducing app bundle size when targeting modern browsers, and allows flexibility in how these polyfills are provided if necessary, which can help fix React Native specific issues, as illustrated in #1799 and #1704 and #1818.

The drawback is that polyfills for these things are now something that client developers will need to think about directly. Shipping Relay to older browsers before this diff should work fine, but after this diff will require global polyfills. As such, this diff may constitute at least a minor-breaking change.
@facebook-github-bot
Copy link
Contributor

@leebyron has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@leebyron
Copy link
Contributor Author

leebyron commented Jun 8, 2017

Alas, because FB's internal environment still asserts a polyfilled Promise during test runs, these tests will fail in that environment even though they succeed in this one.

Closing this PR in favor of #1870, but hopefully this work is not meaningless if we ever change how this internal environment works and wish to support native promises in these tests.

@leebyron leebyron closed this Jun 8, 2017
@leebyron leebyron deleted the no-polyfills branch June 8, 2017 09:13
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.

3 participants