dozoisch / react-async-script

A React composition mixin for loading 3rd party scripts asynchronously
MIT License
177 stars 29 forks source link

Clean up bundled polyfills #26

Closed hartzis closed 6 years ago

hartzis commented 6 years ago

Hello!

While attempting to remove all polyfills for a webpack build and instead use polyfill.io I found that react-google-recaptcha uses this repo which uses babel-runtime.

babel-runtime creates hard polyfill depdencies during transpilation. Example:

var _getIterator2 = require("babel-runtime/core-js/get-iterator");
var _map = require("babel-runtime/core-js/map");

You can see the transpiled code that require's the polyfills here: https://unpkg.com/react-async-script@0.9.1/lib/async-script-loader.js

These polyfills bundled with this library add 21kbparsed and 7.5kb gzipped to bundles and can not be tree-shaken or removed if the same polyfills are already being introduced in the bundle.

Two possible solutions:

  1. clean up/dumb down source code to use different solutions. ie Map -> just plain object?
  2. just transpile source code to es5 and not polyfill. Update the readme to make note of which features the user should polyfill if needed

I'm in favor of the second option and can make a PR to do it 😸 I'm in the boat that thinks packages should not polyfill, but instead let their users polyfill as needed based on their own browser support.

Cheers.

jvivs commented 6 years ago

Would be great to have a way to consume this library in the context of a larger app providing its own polyfills without the duplication.

Some helpful guidance from the W3C Technical Architecture Group on the use of polyfills for library authors can be found here: https://www.w3.org/2001/tag/doc/polyfills/#advice-for-library-and-framework-authors

Original conversation for context: https://github.com/w3ctag/polyfills/issues/6

@dozoisch Thank you in advance for your consideration!

dozoisch commented 6 years ago

@hartzis I think removing Map, and using a plain object might be an easier solution for now, as it won't require anyone to change the way they're using the package. If you are open to make a PR for it, I will gladly review and merge it :)!

Maybe in the future change the way the build is done to output two versions, one with polyfill and one without, but I'd prefer keeping compatibility for now.

hartzis commented 6 years ago

@dozoisch cool. PR #27 made for option 1, just removing Map. I think this should be able to be released as a patch?

dozoisch commented 6 years ago

0.10 is released for react-async-script, and 0.13 is released for react-google-recaptcha!

Thanks @hartzis for the PR 🙌

hartzis commented 6 years ago

@dozoisch thank you for the amazingly active and quick response and follow through! very much appreciated!