search

garycourt / uri-js

An RFC 3986 compliant, scheme extendable URI parsing/validating/normalizing/resolving library for JavaScript
Other
305 stars 69 forks source link

Don't use esnext for module #28

Closed domoritz closed 6 years ago

domoritz commented 6 years ago

Webpack doesn't support es6+ yet.

I don't know what the right thing to do is but I think module shouldn't yet use es6.

garycourt commented 6 years ago

Do you know what specific ES6+ feature that URI.js is using that Webpack is choking on?

domoritz commented 6 years ago

I was running into https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#npm-run-build-fails-to-minify with ajv. I fixed it myself by running babel over uri-js.

vzaidman commented 6 years ago

+1.

please use the es2015 version of this code: node_modules/uri-js/dist/esnext/schemes/mailto.js image

garycourt commented 6 years ago

Ah, ok. I think the better solution would be to create an ES6 dist that is referenced by the module property then.

garycourt commented 6 years ago

I'm not sure if my last suggestion would fix your problem, as for...in has been around since ES5.

Here's the thing, the module property in package.json is for pointing to a version of the code that supports ES6 modules, which is what it is currently doing now. By that extension, the code consuming this file (referenced by this property) must also support ES6 features.

Therefore, any tooling that is using the module property and that does not support ES6 features is in violation of the specification of the module property.

URI.js is using this property correctly, and therefore I can not accept this patch as it would negatively affect those users who are consuming URI.js correctly.

If you are running into this issue, please check that you are running the latest versions of your toolings, and that they support ES6 features, and that they are configured correctly.

domoritz commented 6 years ago

Thank you for looking into this! I think I understand the properties better now. module is a not yet standardized property and it's supposed to point to code that uses es2015 import syntax but es5 code otherwise (see https://github.com/rollup/rollup/wiki/pkg.module#wait-it-just-means-import-and-export--not-other-future-javascript-features). I think the right thing is to leave everything as you have it right now (i.e. use es2015 imports) but change https://github.com/garycourt/uri-js/blob/master/tsconfig.json#L4 to es5, right?

vzaidman commented 6 years ago

you are both right. i just want to point out something small about

for...in has been around since ES5.

not with const. as i showed above, ie11 doesn't supports for in with consts.

it supports consts and it supports for in but not the combination of the two.