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

Object.values and Array.find written with older browser support in mind #11

Closed
wants to merge 1 commit into from

Conversation

mfedderly
Copy link

After removing the polyfills, I suspect that calls to Object.values and Array.find will be broken in some browsers. Object.values is a developing spec, and Array.find isn't in IE11. These are hand-written replacements that should work well enough for the usage here. Also added some tests for the getAll* methods.

@meikidd
Copy link
Owner

meikidd commented Dec 8, 2017

I think that most people use babel to precompile their projects or use webpack/gulp to bundle their modules, in case that babel and webpack/gulp will deal with the es6 code, so remove babel-polyfill is safe.

If we aim to provide this package to let people use directly in browser without precompiled, there will be much more things to do such as replacing es6 import/export with commonjs import/export.

Could you pls show me how you import this package in your project and how to build your project?

@mfedderly
Copy link
Author

Right now I'm using typescript compiled to es5 and webpack and no babel. Webpack handles the es6 modules during its transform. On top of that, I have a set of shims that I'm manually including right now. Although I have plans to move to babel, I don't know if other consumers of this library will necessarily expect to have to polyfill those two methods to pick up the new release. With this PR, no consumers should experience browser breaks.

@meikidd
Copy link
Owner

meikidd commented Dec 8, 2017

I thought most people use babel-loader as their webpack module loader. I've changed babel to webpack for compiling the package and provide a umd module that can be used in both browsers(include old browser) and node.js. Maybe you can pull this down and try it.

pls let me know If this works, then I will publish a new version. thanks.

@mfedderly
Copy link
Author

The webpack approach is interesting but it has a few drawbacks. The bundled size is 40k now (15k before), and all of the polyfills from babel cannot be deduplicated within my own build. Typically when I build a consumable library I publish it as node-style modules (require statements) and then let the consumer deal with webpacking. I am specifically not trying to use this directly in a browser, since I've got my own webpack step. Recently I've taken the additional step of also providing es6 modules in a different directory and referencing that with "modules": "es" in my package.json. That way tools that support es6 modules can use the new style.

In either case, I treat the module syntax as separate from the browser compatabilty of the actual code, if that make sense.

@meikidd
Copy link
Owner

meikidd commented Dec 12, 2017

Typically when I build a consumable library I publish it as node-style modules (require statements) and then let the consumer deal with webpacking.

There are alternative ways, we should make a choice

  • the package provide only source code, let the consumer deal with the es6+ syntax.
  • the package provide precompiled code that can be used in most enviroment.

If we choose the first way, polyfills will not be included since babel is more professional.

@meikidd
Copy link
Owner

meikidd commented Dec 12, 2017

How about this,

if you want small size and babel polyfills are reused in your project, import the package like this

import ISO6391 from 'iso-639-1/src/index'

if you need precompiled code that are more compatible, import like this

import ISO6391 from 'iso-639-1'

@mfedderly
Copy link
Author

I think the preferred solution to having two module formats is to have two entries in your package.json to allow your library to be consumed in both ways. Webpack supports this correctly and will consume the library as an es6 module and not the built version.

I think its probably reasonable to assume that if someone is consuming es6 modules, they are applying their own polyfill layer as well.

"main": "build/index.js",
"module": "src/index.js"

@meikidd
Copy link
Owner

meikidd commented Dec 13, 2017

I have added the module field and published version 2.0.2. Thank you very much.

@mfedderly
Copy link
Author

That looks great. I'm going to go ahead and close this.
Thanks for all the help!

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.

2 participants