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

index.es.js should be in ES5 + ESModules #57

Merged
merged 2 commits into from
Nov 21, 2018
Merged

index.es.js should be in ES5 + ESModules #57

merged 2 commits into from
Nov 21, 2018

Conversation

gfx
Copy link
Contributor

@gfx gfx commented Nov 2, 2018

This is because webpack's default resolution of main fields is:mainFields: ['browser', 'module', 'main'], which means index.es.js is selected by webpack even if it targets older IEs.

https://webpack.js.org/configuration/resolve/

I've set its value to mainFields: ["browser", "main", "module"] for now in my project, but I think the "module" entry point should be written in ES5 + ESModules, not ES2015 + ESModules.

This is because webpack's default resolution of main fields is:
`mainFields: ['browser', 'module', 'main']`
which means index.es.js is resolved by webpack even if it targets
older IEs.

https://webpack.js.org/configuration/resolve/
@yahoocla
Copy link

yahoocla commented Nov 2, 2018

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@gfx
Copy link
Contributor Author

gfx commented Nov 2, 2018

Agreed to CLA. Thanks.

@coveralls
Copy link

coveralls commented Nov 2, 2018

Coverage Status

Coverage remained the same at 98.936% when pulling e11b6aa on gfx:es5+esm into 29eea1c on yahoo:master.

@redonkulus
Copy link
Collaborator

Isn't it implied that if a browse supports es modules, that it should support ES6+ syntax?

@gfx
Copy link
Contributor Author

gfx commented Nov 3, 2018

Currently the benefit of providing ESModules for browsers is the tree-shaking by webpack, means webpack downpiles ESModules syntax even if no transpiler (e.g. Babel) is configured.

https://webpack.js.org/guides/tree-shaking/

It relies on the static structure of ES2015 module syntax

As far as I know, most of projects that use webpack are configured not to transpile external libraries, so libraris should be compatible with ES5 if the projects support legacy IEs, except for ESModules syntax because the syntax is transpiled by webpack.

Thus, if a library has "module" field for browsers, it should be ES5 + ESModules. it may feel strange, though.

@JonatanGarciaClavo
Copy link

My 2 cents, i detected problems related to that in old Safari browsers that doesn't support const. So this will help me a lot, well it will be expected behaviour if you use this with webpack as @gfx mention before.
Also should be modified following line
const IS_CLIENT = typeof window !== 'undefined';
to be
var IS_CLIENT = typeof window !== 'undefined';

@gfx
Copy link
Contributor Author

gfx commented Nov 10, 2018

@JonatanGarciaClavo Thanks! Fixed const in e11b6aa

FYI If you use webpack, https://github.com/bitjourney/check-es-version-webpack-plugin should be useful to detect ES2015 in your bundles.

@JonatanGarciaClavo
Copy link

@gfx wow thanks for the plugin i didn't know about it, i will use it because my client use IE 11 as supported browser 😞 , that will help me to detect this case in a easy way, not only when i see failure report from app insights hehe.

@JonatanGarciaClavo
Copy link

@redonkulus so is there any problem to make this happens? Or are we missing something that can't make it happens?

@redonkulus
Copy link
Collaborator

I'm ok with merging this, sorry it took so long.

@redonkulus redonkulus merged commit 51bc15d into yahoo:master Nov 21, 2018
@redonkulus
Copy link
Collaborator

released in v2.0.5

@gfx
Copy link
Contributor Author

gfx commented Nov 30, 2018

Thanks!

@gfx gfx deleted the es5+esm branch November 30, 2018 01:33
@roderickhsiao
Copy link
Contributor

Thank you!

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.

6 participants