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

Implement babel-loader in webpack (with polyfill) #379

Closed
wants to merge 1 commit into from
Closed

Implement babel-loader in webpack (with polyfill) #379

wants to merge 1 commit into from

Conversation

tavogel
Copy link
Contributor

@tavogel tavogel commented Oct 21, 2019

Fixes #371

Uses core-js v3 polyfills

Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks for the contrib. I'm curious if there's something we could add to our web tests to ensure that this doesn't regress.

@@ -73,5 +76,7 @@
"hystrix",
"rate-limiting"
],
"dependencies": {}
"dependencies": {
"core-js": "^3.3.3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a devDependency shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope! More info:
https://babeljs.io/blog/2019/03/19/7.4.0

The core-js package is a light suite of polyfills for all the major new JS features. With babel useBuiltIns, it will dynamically add the imports for any core-js polyfills to the dist folder. These polyfills will bloat the distribution a tiny bit, but this package will be able to serve a much wider audience as a result. If you prefer to leave the polyfills out, you can remove coreJS and just use babel preset-env, with no args, to transpile only the syntactic sugar, without adding any polyfills. Given that this package is marketed for browsers, I think the overhead is worth it.

Note there's also a second regenerator-runtime polyfill dependency that Babel can generate imports for. I didn't include it as a dependency in this PR because you don't use generators or async/await code. If you would prefer maintaining async/await code, then you can easily add the regenerator-runtime dependency and a single require("regenerator-runtime/runtime"); in your entrypoint (like described in the babel blog). This will transpile your bleeding edge ES2019 into browser-native ES5 at build time.

@tavogel
Copy link
Contributor Author

tavogel commented Oct 21, 2019

For regression testing, I guess you could do something with Selenium/IE11, BrowserStack, etc. Another possibility would be piping the output through the Webpack UglifyJS plugin, which only supports ES5 bundles. This would give you an added benefit of shrinking your dist size. But then you need to test that the Uglified output works too... :)

@lholmquist lholmquist self-requested a review October 21, 2019 23:16
@tjenkinson
Copy link
Contributor

What actually needs a polyfill? My view is that libraries shouldn’t do any polyfilling and instead leave it up to the developer. Polyfills are modifying globals which is generally a bad idea.

Imagine an app becomes dependant on a polyfill in here for example without realising, and then they make a change to lazy load opossum. There’s the risk of their app breaking depending on when the polyfill gets loaded in, which could also be difficult to debug.

So my preference would be no polyfill and a note about required polyfills and maybe some example code to add them, or even better rewrite the logic that needs a polyfill to no longer need one (if feasible)

@lholmquist
Copy link
Member

Is there something that needs a polyfil specifically? I think we tried really hard to make this library have no dependencies, so it would be a bummer if we added a dependency to it just for 1 thing

@tavogel tavogel changed the title Implement babel-loader in webpack Implement babel-loader in webpack (with polyfill) Oct 24, 2019
@tavogel
Copy link
Contributor Author

tavogel commented Oct 24, 2019

Added an alternative PR with no polyfill: #380

The polyfills installed by babel/core-js are:

es.array.concat
es.array.for-each
es.array.from
es.array.is-array
es.array.iterator
es.array.reduce
es.array.slice
es.array.sort
es.date.now
es.date.to-string
es.function.name
es.number.constructor
es.number.is-integer
es.number.max-safe-integer
es.object.create
es.object.define-property
es.object.get-prototype-of
es.object.keys
es.object.set-prototype-of
es.object.to-string
es.promise
es.regexp.exec
es.regexp.to-string
es.string.iterator
es.string.replace
es.symbol
es.symbol.description
es.symbol.iterator
es.weak-map
web.dom-collections.for-each
web.dom-collections.iterator
web.timers

It's a huge list because no browserlist was defined. If you put bigger limits on the browsers you target, you get far fewer polyfills.

You can compare the differences between the /dist/opossum.js files when you run npm run build in the branch for each PR. Quite a big difference!

I agree that since you already have no deps, it should probably stay as lean as possible.

@tavogel
Copy link
Contributor Author

tavogel commented Oct 24, 2019

Closed for #380

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IE 11 support / babel
4 participants