serviejs / popsicle

Simple HTTP requests for node and the browser
MIT License
246 stars 19 forks source link

Popsicle needs polyfills because of ES6 features #86

Closed adrien-gueret closed 7 years ago

adrien-gueret commented 7 years ago

Hi!

First of all, this is a very good library with awesome API, thanks for the job.

I just face a small issue when using Popsicle with PhantomJS or on not-so-old browser like IE10: the library uses the ES6 method Object.assign and since TypeScript does not "compiled" it into ES5-friendly code, Popsicle simply crashes.
We have a similary with Promises.

The workaround is simple, we just need to add polyfills for Object.assign and Promises and everything is OK.

But... I feel it's quite sad that a developer needs to include polyfills if he wants to use a library.

I don't know what is your feelings about it?

blakeembrey commented 7 years ago

It also uses promises, so some ES6 features are required since 9.0. I can make a note of what needs to be included in the README - it's currently just those two. I used to use any-promise and xtend, but recently updated to reduce the dependency footprint by using standardised APIs.

blakeembrey commented 7 years ago

By the way, I think it is a good idea to use the standardised APIs where possible. I previously supported node 0.10 with popsicle, which was the reason for xtend and any-promise. I understand there's still browser environments older the node 0.10, but supporting them can be a big ask when they are at varying levels of support for APIs in use (e.g. if you're supporting browsers without promises, there's no chance I might break something with an obscure use of ES5 APIs). I think it's more beneficial to reduce the build footprint and let bundlers select if they want the polyfills so they can either include them for everyone (so each module doesn't have a dependency on the dozen "extend" modules). Maybe babel could be used here for the final build? They have an interesting preset called env which I actually think is brilliant here as babel does support runtime polyfills and could work more efficiently with dependencies.

adrien-gueret commented 7 years ago

Looks good for me, thanks for your very quick reply :)

mboudreau commented 6 years ago

I have to say that I don't agree with this decision. I'm trying to include popsicle because we have a need to have it work for both node and browser, but all of these are making it more difficult than need be. If you're concerned about footprint, then create 2 different versions of the distribution, one with the polyfill and one without, however, I would recommend you just have the polyfill included since your whole schtick is "Simple HTTP requests for node and the browser" which is simply not true if I have to add polyfills, it should just work. If people were concerned with footprint, they should just build their own or find another package that's smaller. You also really don't need to include xtend since you could code it easily yourself in a few lines with a for in.

There's nothing simple of adding 2 polyfill in my project dependencies, just to make it work with a project dependency that's suppose to fix this very issue...