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 #170

Closed
ForbesLindesay opened this issue Jul 25, 2016 · 43 comments
Closed

Polyfill #170

ForbesLindesay opened this issue Jul 25, 2016 · 43 comments
Milestone

Comments

@ForbesLindesay
Copy link
Contributor

Polyfilling via babel doesn't work very well in my experience since you don't normally run babel against node_modules. For apps (rather than libraries) I think it's generally best to just have global polyfills.

With this in mind, we could consider just adding a script to link to https://polyfill.io/v2/docs/ as the default polyfill. It provides a polyfill based on the user agent string so is a decent future proof default.

For non-polyfill babel-runtime components, we should do as @insin suggest in #51 (comment)

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

Are any other major tools using/recommending this?

@ForbesLindesay
Copy link
Contributor Author

Not that I'm aware of. Their usage stats seem fairly impressive (https://polyfill.io/v2/docs/usage). I'm not averse to us offering a different (e.g. static) js polyfill, I just think we should default to including a global polyfill of some sort.

The least controversial thing might be to just provide a static polyfill (not sure which ones are popular these days). I think we should provide something though as soon as possible.

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

I would normally include core-js but it is way too large. I agree it’s better to ship a polyfill than not. I’ll ask for feedback on Twitter I guess.

@ForbesLindesay
Copy link
Contributor Author

We could ship full core-js in development build, then build a custom core-js based on analysing code for which features are actually used in production. Doing that analysis in development would probably be way too slow since it would require analysing all of the dependencies.

We could also look at building a couple of different bundles, one for browsers with full ES5 support and another for browsers that need ES5 polyfills, then feature detect on the client.

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

Feedback about polyfill.io seems generally positive.
I think we can get it in.
(Worst case: we’ll remove it later.)

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

Do we have a good solution to this? polyfillpolyfill/polyfill-service#561

@apaleslimghost
Copy link
Contributor

@gaearon we're fixing that soon

@triblondon
Copy link

I will ship a fix for polyfillpolyfill/polyfill-service#561

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

What is the planned fix? Is it future proof?

@apaleslimghost
Copy link
Contributor

@ForbesLindesay
Copy link
Contributor Author

I wonder if a better fallback would be to hedge slightly and deliver at least the most commonly needed polyfills, rather than delivering no polyfills.

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

I don’t understand the problem space that well, would the proposed fix also work for Twitter / Slack / LinkedIn / ...?

@gaearon gaearon added this to the 0.2.0 milestone Jul 25, 2016
@apaleslimghost
Copy link
Contributor

The proposed fix is to adapt the existing UA aliasing so it's not just mapping between browser families but so it can alter the entire parsed UA. We could then do something like:

'facebook': function(ua) {
  return {family: 'ios_saf', major: ua.os.major};
}

This would easily extend to other in-app browsers. (Twitter at least looks like it's correctly detected as MobileSafari)

@triblondon
Copy link

We lean on ua-parser, because life's too short to write your own UA classification lib! However, sometimes its classifications are not useful for us, and we alias them.

@gaearon
Copy link
Contributor

gaearon commented Jul 25, 2016

I think the lack of unhandled rejection reporting is unfortunately a blocker for us. This is a super common problem with React. People call setState() in fetch callback, their render() method throws, they have no idea what’s going on, and React gives them unhelpful errors because it is now in an inconsistent state.

Would you be interested in adopting a Promise polyfill that reports unhandled rejections by default? I believe core-js (shipping with Babel) does it.

@insin
Copy link
Contributor

insin commented Jul 26, 2016

How about including a default Promise polyfill which ensures everyone gets a version which avoids that issue?

I like @mjackson's solution for this in his web-starter project, providing static minified versions and loading them only when needed:

<script>window.Promise || document.write('\x3Cscript src="/es6-promise.min.js">\x3C/script>\x3Cscript>ES6Promise.polyfill()\x3C/script>')</script>
<script>window.fetch || document.write('\x3Cscript src="/fetch.min.js">\x3C/script>')</script>

I think you can do something similar with Webpack's import and export loaders, but don't quote me on that.

@gertjvr
Copy link

gertjvr commented Jul 26, 2016

We had to remove polyfill.io due to it failing to identify ie11 correctly.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

OK, thanks for feedback.
I think we won’t push it as the default solution then.

What @insin suggested in #170 (comment) looks super reasonable to me. Mostly because fetch and Promise are two features that are easy to miss in a polyfill (or to get a bad polyfill).

So I think we should go with that approach instead.

@apaleslimghost
Copy link
Contributor

@gertjvr could you raise that at Financial-Times/polyfill-service please? Seems like a fairly major issue.

@gertjvr
Copy link

gertjvr commented Jul 26, 2016

@quarterto will do its not polyfill.io issue actually its to do with useragent npm package but didn't dig to deep.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

The reason I’m inclined to not use polyfill.io right now is that most React users don’t care about ES6 features like Maps etc, so if they want a polyfill, they can add it themselves. However they do need two polyfills: fetch and Promise. And we’d like to have control over which Promise we ship because it’s so easy to get it wrong.

@mjackson
Copy link
Contributor

mjackson commented Jul 26, 2016

The best way to do this IMO is to do a client-side feature test and a document.write to get the polyfill if you need it. This way, you can avoid putting the file in your bundle altogether and do one less HTTP request on modern browsers.

This is actually one of the primary reasons I need to server-render my template in web-starter. So I can do those polyfills.

If server-rendering is off the table (note that I'm not talking about server-rendering your whole app-just the "chrome" and a bunch of <script> tags) then I'm not sure the best way to do this.

Please note, however, that in order to use the WebpackManifestPlugin properly (#210) we'll need to have control over how the page is rendered anyway, because we need to do the same trick, so it might be worth looking into.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Hmm, can you help me understand why server rendering is important here? Couldn’t we just put these conditional document.writes into our index.html template?

@ForbesLindesay
Copy link
Contributor Author

We can just put them in our index.html template. The downside is that it gets exposed to the user and becomes impossible for us to update because the template lives in the user's repository.

A better solution I think might be to add something like {{polyfills}} to the template and then use a loader (see https://github.com/ampedandwired/html-webpack-plugin/blob/master/docs/template-option.md) to add in the scripts.

I don't think "server rendering" is actually relevant here, so much as we just need some way to add the appropriate inline scripts to the page.

@mjackson
Copy link
Contributor

Ah, all I meant by "server rendering" was that you need to be able to affect the HTML. It sounds like index.html will work beautifully.

@taion
Copy link

taion commented Jul 26, 2016

babel-polyfill isn't that large.

A gzipped, minified build with just react + react-dom is 41 kB. Adding babel-polyfill only takes that to 69 kB.

Without gzipping, it's 138 kB v. 222 kB.

Certainly there's room to optimize, but I don't think "adding 28 kB to the gzipped size" is sufficient for the easiest solution to be dismissed outright as "way too large".

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

We’d also need fetch.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Also, React gets a lot of bad rep for being “large”. We can say that 40kB gzipped is less than many images but that’s what people look at in comparisons, and I can guarantee that if we make it 70kB by default, somebody will use this in charts claiming React apps are huge (especially considering the “default” status of this tool).

So we should be careful bundling extra stuff. But perhaps bundling Promise, Symbol from core-js and fetch is good enough.

@taion
Copy link

taion commented Jul 26, 2016

71 kB gzipped, 228 kB without gzip if I add isomorphic-fetch above.

I think didactically, in this context, it's just better. One-line polyfill import (and one that ships with Babel, at that) v. something non-trivial to ship exactly the correct polyfills.

The other thing the React ecosystem gets a bad rep for is tooling complexity, and doing bespoke, advanced stuff to pull in specific polyfills I think hurts more on the complexity front than it helps on the bundle size front.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

OK. Here’s my proposal. Let’s ship these polyfills:

  • whatwg-fetch
  • Promise and Symbol from core-js
  • Regenerator runtime if you use generators or async (we’d need to add support for async too)

We would bundle them for everybody via entry config. It would not be configurable.

@taion
Copy link

taion commented Jul 26, 2016

I don't actually know how to import core-js polyfills individually. 2 minutes of guessing gives me:

import 'react';
import 'react-dom';
import 'core-js/es6/promise';
import 'core-js/es6/symbol';
import 'isomorphic-fetch';

This gives me 49 kB gzipped, 161 kB just minified. (These numbers are a bit weird since I'm using webpack 2... maybe... probably not too bad.)

I don't know. I really like Object.entries, Array.prototype.includes, &c.

The other option is to use babel-runtime, which effectively will only polyfill things that are used. It won't polyfill e.g. Array.prototype.includes (but the proposal above doesn't either).

It will also reduce bundle size a bit by using external helpers from babel-runtime.

The main downside that I'm aware of is that the transform is paranoid to the point of also trying to polyfill e.g. Object.keys, and that there's no way to disable that separate from disabling all polyfilling.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

I don't know. I really like Object.entries, Array.prototype.includes, &c.

They’re nice but from my experience with answering React issues beginners don’t really use them / know about them, and those who do know, know how to include a polyfill.

On the other hand, Promise causes a ton of problems because people use a bad polyfill that swallows errors, Symbol causes confusion because some optimizations require it, and fetch is just easy to forget accidentally.

@taion
Copy link

taion commented Jul 26, 2016

I agree that adding 8 kB of polyfills to get a mostly working ES6 environment is a strict win.

I think the babel-runtime approach is worth investigating, assuming it's feasible to make a babel-runtime that doesn't try to polyfill Object.keys and other ES5 things.

You'd get the extra functionality if you needed it, and you'd only pay in bundle size for features that you actually use. The extra complexity is just a single additional Babel plugin.

(Also, selfishly, if there were a babel-runtime that didn't polyfill silly things, I'd use it everywhere – right now it's a size penalty on small-ish libraries because it tries to polyfill things that don't need to be polyfilled.)

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Would you like to take a stab at implementing my proposal from #170 (comment)?
We can leave regenerator/async out of scope for the first iteration of this.

I think the babel-runtime approach is worth investigating, assuming it's feasible to make a babel-runtime that doesn't try to polyfill Object.keys and other ES5 things.

Agree, in the future.

@ForbesLindesay
Copy link
Contributor Author

The issue with babel-runtime is that it doesn't polyfill things that are used by dependencies in node_modules. Increasingly (with the popularity of node v4/v5/v6 starting to overtake 0.10) people will start shipping modules to npm that depend on things like Map and Set being implemented. It doesn't make sense for all of those modules to ship with separate polyfills for Map and Set. Polyfills just don't work properly if they're not global.

I'm keen for us to implement @gaearon's solution in the short term. In the longer term, we can make something clever enough to load only the required dependencies, but do so globally for all modules.

@taion
Copy link

taion commented Jul 26, 2016

@ForbesLindesay The current proposal isn't polyfilling Map or Set anyway, though. In general I think library authors who use post-ES5 features other than maybe Promises or fetch should be using babel-runtime. It's what I try to do, anyway.

@gaearon I'm not sure I'll have time to look at it right now. I don't even know for sure that I imported those polyfills correctly. I'd prefer someone who actually has worked with core-js directly do this.

@ForbesLindesay
Copy link
Contributor Author

The current proposal isn't polyfilling Map or Set anyway

True. I'm trying to come up with a solution that does let us start using those features. In the short term, library author's may have to bundle polyfills for them, but we have an opportunity to change that by providing an efficient polyfill that's only provided if the code needs it. We can easily statically analyse code to detect whether or not it uses Set and Map, just look for a global variable of that name. If we provide polyfills only to apps that use them and only to browsers that don't already have them built in, that's a strictly positive thing. It will also save a lot of pain that I've personally felt when deploying an app that has been mostly tested in chrome latest.

@triblondon
Copy link

IIRC, using document.write to add polyfills can be a bad idea because:

  1. It's not future friendly: the upcoming Feature API will include controls to disable document.write in a CSP-like way
  2. A document.write will block the preloader and restart the parser, resulting in a slower first render

Of course the benefit of the polyfill service is that you get the feature detection done server-side, so you don't need to run JavaScript to figure out whether you need to load other JavaScript.

Our promise polyfill seems to serve most users' needs but it is not perfect, and we'd welcome a submission of a better one.

@ForbesLindesay
Copy link
Contributor Author

We can get round document.write anyway because we control the entire stack. We could add code directly to the bundle to do the feature switching and document.body.appendChild if that turns out the perform better.

Re promise polyfills, I would suggest the promise npm module. Once then/promise#129 is merged you'll be able to simply require('promise/es6') to get only the standards compliant helper methods. I've spent a lot of time optimising it and it's the implementation we use at Facebook.

@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

#235

@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

For now #235 covers fetch and Promise.
We can revisit this in the future and add more polyfills on a case by case basis.

@gaearon gaearon closed this as completed Jul 27, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

This is shipped in 0.2.0 alpha, please feel free to provide feedback. Here's how: #190

@wefiy
Copy link

wefiy commented Mar 20, 2018

mark

@facebook facebook locked and limited conversation to collaborators Mar 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants