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

Polyfill features for React 16 #3200

Closed
wants to merge 2 commits into from
Closed

Polyfill features for React 16 #3200

wants to merge 2 commits into from

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Sep 27, 2017

Looks like these polyfills add about 6.26 KB (gzipped) to the bundle.

Total bundle reduction from React 15 to 16 (+ polyfills) amounts to 1.82 KB, though! That's better than an overall increase. 🎉

@react-scripts-dangerous

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit 9f1f193) has been released on npm for testing purposes.

npm i react-scripts-dangerous@1.0.15-9f1f193.0
# or
yarn add react-scripts-dangerous@1.0.15-9f1f193.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.15-9f1f193.0 folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2017

We should find smaller polyfills IMO. This is too much.

@abdennour
Copy link

I totally agree @gaearon ... What about using core-js ?

@gaearon
Copy link
Contributor

gaearon commented Sep 27, 2017

That's the one we use in this PR, and it is too large. We need to find smaller shims.

@zikaari
Copy link
Contributor

zikaari commented Sep 27, 2017

In production can't we use something like https://polyfill.io or similar real-time solution?

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2017

You can use any polyfill service you'd like, but I suggest you polyfill manually and only what you need.
Polyfills are large and slow down your time-to-first-render.

Currently, we support IE 9 so we will polyfill everything we require to run on IE 9.

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2017

So, while looking for alternative Map/Set implementations, it seems to me that most alternatives are O(n), not O(1).

Also, if we don't polyfill with a full spec-compliant polyfill, users may run into issues bringing their own polyfills since they wouldn't overwrite ours (in normal cases).

I think core-js is the way to go as it's the de facto standard (remember things like Symbol & Iterator are coming along for the ride too, so for-of works now in IE9).

@Timer
Copy link
Contributor Author

Timer commented Sep 27, 2017

We can polyfill strictly Map & Set for 899 B (sub 1KB).

This does not polyfill Symbols or Iterators (but returns a fake iterator instead).

@gaearon
Copy link
Contributor

gaearon commented Sep 28, 2017

@Timer
Copy link
Contributor Author

Timer commented Sep 28, 2017

Yes, it's deprecated out of interest rather than functionality afaik; and the polyfill suggested to be used is comparable to core-js.

It's the only polyfill I found which doesn't polyfill Iterator and Symbol as well.

@Timer
Copy link
Contributor Author

Timer commented Sep 28, 2017

See #3208, too.

@GreenGremlin
Copy link
Contributor

Why not use feature detection and require.resolve to conditionally load polyfills? Webpack has a simple example of how to do this in their documentation.

@GreenGremlin
Copy link
Contributor

GreenGremlin commented Oct 10, 2017

Something like this, maybe?
GreenGremlin@0f82e5e

edit: My groupings of polyfills is definitely off

@Timer
Copy link
Contributor Author

Timer commented Oct 11, 2017

IMO we're not going to do much better without making tradeoffs. I'd like to merge this as-is and seek smaller polyfills.

This will become less of an issue in v2 when polyfilling can be removed all together when targeting newer browsers only; we're sort of hampered by the IE9+ support in v1.

Remember we can even drop IE 9 and 10 support in v2, supporting only 11 which has everything necessary for React.

@Timer Timer added this to the 1.0.x milestone Oct 11, 2017
@zikaari
Copy link
Contributor

zikaari commented Oct 26, 2017

Maybe relevant context -> #1249 (comment)

@gaearon
Copy link
Contributor

gaearon commented Oct 28, 2017

I decided against this. When people upgrade to React 16 they make an informed decision (it's prominently called out in release notes and changelog). We should probably link to high quality polyfills but that's the extent of it.

That said we need to re-evaluate what we support out of the box now. If some browsers fell away due to React 16, maybe we can remove polyfills that were needed only for those browsers.

@gaearon gaearon closed this Oct 28, 2017
@Timer Timer deleted the react-16 branch October 28, 2017 20:59
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants