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

Include polyfill.io in HTML by default #1104

Closed
gaearon opened this Issue Nov 27, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@gaearon
Member

gaearon commented Nov 27, 2016

We considered doing it earlier but settled on shipping just Object.assign, Promise and fetch polyfills by default.

I'd like to reconsider adding a polyfill.io scripts to HTML as too many people rely on ES6 features without realizing they need polyfills. And using polyfill.io means it's easy to opt out too if you know what you're doing.

We should only include ES6 polyfills, and specifically exclude Object.assign, Promise, and fetch because we already have them.

@Timer

This comment has been minimized.

Collaborator

Timer commented Nov 30, 2016

I would argue against adding a library which is fetched from the internet.
For example, I deploy applications in environments without an internet connection where the polyfill couldn't be accessed. I am now in a position where I wrote code which assumes the polyfills exist but do not.

@gaearon

This comment has been minimized.

Member

gaearon commented Nov 30, 2016

It's in index.html so you can always remove it. Would that not be a good default for most users?

@Timer

This comment has been minimized.

Collaborator

Timer commented Nov 30, 2016

I feel like it would be a fair default most of the time for a CRA user.


I guess my only other complaint is still that the polyfill relies on an external service -- so what happens when polyfill.io's CDN goes down or has a DNS error/etc?
There will be script errors and application errors which are cryptic to the user of the application (in production) and other unexpected consequences.
I believe forcing (or rather, encouraging) users to include polyfills on a feature-by-feature basis ensures they're bundled into the bundle.js and are always available when the application is available.

Sure, advanced users can remove polyfill.io and include feature-polyfills. So the question becomes is something that only works 99% of the time good enough for the non-advanced user?

I would feel more comfortable using babel-polyfill (I know it's heavy), but maybe we can make some improvements on that side of things using a locally-ran service similar to polyfill.io.

edit: fixed typo

@robcaldecott

This comment has been minimized.

robcaldecott commented Nov 30, 2016

Relying on a 3rd-party service for this seems a little risky imho. I'd rather see a configurable build warning for missing polyfills, or even a way to specify target browsers.

@tbillington

This comment has been minimized.

tbillington commented Nov 30, 2016

Why have half the polyfills baked in and half as 3rd party service, does react-scripts rely on the baked in ones?

@gaearon

This comment has been minimized.

Member

gaearon commented Dec 1, 2016

I'd rather see a configurable build warning for missing polyfills

We can't determine them statically.

or even a way to specify target browsers.

This sounds reasonable and actually something we're already planning to enable.
So we might as well wait for babel/babel-preset-env#51 and babel/babel-preset-env#20.

@Grsmto Grsmto referenced this issue Dec 1, 2016

Closed

Use polyfill.io ? #34

@cfenzo

This comment has been minimized.

cfenzo commented Dec 8, 2016

Please don't include polyfill.io by default..

All though I love the idea behind polyfill.io, we've had some mixed experiences using it and in the end we banned it completely from all projects/products.

In addition to the bit about relying on a 3rd party plugin loaded over the net,
you can't overlook the part of it being based on UA-sniffing. And despite their great effort, there will always be a risk of unmatched browsers.
And an unmatched browser === failing app.

And when you run in to that situation in production, where your site/app isn't working for some users, and your client/customers/users are calling (or worse, just leaving), and debug hell is loose.. It's not a good day to be a developer.
And it's certainly not a situation you want the non-advanced users of CRA to experience...

@gaearon

This comment has been minimized.

Member

gaearon commented Dec 8, 2016

Fair enough. I think babel/babel-preset-env#20 will ultimately solve this.

@gaearon gaearon closed this Dec 8, 2016

@cdaringe

This comment has been minimized.

cdaringe commented Jan 11, 2017

looks like 20 ==> babel/babel-preset-env#56 which merged.

at this point, reading through the issues, i'm not sure where we stand with what is supported and what is not. i see that my app crashes for lack of Symbol support in IE11, so clearly we haven't included that polyfill yet. I'd be happy to write some docs, but I figure the maintainers probably have a better grip on where this stands as it is, ATM

@gaearon

This comment has been minimized.

Member

gaearon commented Jan 11, 2017

i see that my app crashes for lack of Symbol support in IE11, so clearly we haven't included that polyfill yet.

The default app shouldn't crash. Only if you use Babel features that rely on this polyfill.

at this point, reading through the issues, i'm not sure where we stand with what is supported and what is not.

We currently polyfill Object.assign, Promise and fetch, nothing else.

do we need to update anything for babel-preset-env?

You can track #1249 for progress. It will take a while to get this sorted so in the meantime a doc PR for polyfills and caveats (e.g. for of requiring a polyfill in some cases) is welcome.

@cdaringe

This comment has been minimized.

cdaringe commented Jan 18, 2017

I didn't include a caveat section in #1404 because I'm not sure how to generate a deterministic set of features that don't get transpiled or polyfilled. if i was able to generate that list, its value is still limited without a supporting cross-ref table showing those features against a can-i-use inspired table of browsers. that would be saaawweeet 🌮. too much effort ATM!

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