Add ES module version #190

blikblum · 2019-06-24T15:54:06Z

Fixes #174

Add an build step to create an es module version and configure to be run before publish.

The created file index.esm.js is not committed since is derived from main source

Configure module entry in package.json to point to index.esm.js

coveralls · 2019-06-24T15:58:18Z

Coverage remained the same at 100.0% when pulling a913bb8 on blikblum:create-esm into d1ec03a on pillarjs:master.

wesleytodd · 2019-06-24T16:24:22Z

Hi @blikblum, I personally am a 👎 on this change. Node does not yet support ESM w/o a flag, and the current unstable support uses "type": "module" or .mjs to specify ESM, which is not what this implements.

If your build tooling does not support the most popular module format, choose different tooling?

blikblum · 2019-06-24T16:35:07Z

Node will still use the commonjs version. Only webpack and rollup will load the esm version

wesleytodd · 2019-06-24T17:13:32Z

Yes I understand that. The point is that a library making changes to support non-standard things for the tool of the day is not a good way forward. The impact of adding something like this to such a popular package is large and will last for years. IMO it is just not a good path to take.

paulmelnikow · 2019-06-24T17:51:07Z

index.js

+pathToRegexp.parse = parse
+pathToRegexp.compile = compile
+pathToRegexp.tokensToFunction = tokensToFunction
+pathToRegexp.tokensToRegExp = tokensToRegExp


Other arguments against this notwithstanding, in the esm version, these should be separate exports, not properties of the default export.

Making named exports would be a breaking change.

But if necessary i can do it

blikblum · 2019-06-24T18:07:43Z

@wesleytodd

The point is that a library making changes to support non-standard things for the tool of the day is not a good way forward

ES module is standard, and i cited webpack and rollup that is used often, but this change will allow to the library be used by chrome without bundling. This is the reason i created this PR.

So its walking in the direction of the standard and making the "tools of the day" optional, not the opposite

wesleytodd · 2019-06-24T18:20:04Z

@blikblum, but it is neither using the standard nor is it following the experimental implementation for Node. If you wanted to follow the ECMA standard you would use the export keyword, and if you wanted to follow the Node experimental ESM implementation you would name the file .mjs. Neither of which you are doing.

I guess I could get on board with an implementation which used .mjs. If you wanted do do that I am not sure why you would not just write a transform which properly creates a spec compliant file with exports.

blikblum · 2019-06-24T18:29:06Z

ECMA standard you would use the export keyword

Based in what you are saying that? It uses export keyword instead of module.exports. Did you run the script to see the generated file?

ESM implementation you would name the file .mjs

I can change the extension to mjs. No problem. Its a 5 byte change. But this is supposed to be used by browser not by node because its current experimental in node but in production on browsers

wesleytodd · 2019-06-24T18:34:02Z

Based in what you are syaing that? It uses export keyword instead of module.exports. Did you run the script to see the generated file?

#190 (comment)

@paulmelnikow's comment is what I was referring to.

current experimental in node but in production on browsers

Popular modules like this need to consider the long term future before making changes. If the file generated file is not .mjs and that is what node expects in the future, then it would be a breaking change to change later.

Personally, until the experimental flag is removed I am not a fan of using ESM at all, but I can see how that answer does not sit well with some folks. So if there is a path forward which both works in modern browsers and also is compliant with the experimental version I would compromise my opinion.

blikblum · 2019-06-24T18:40:13Z

#190 (comment)

@paulmelnikow's comment is what I was referring to.

As i pointed earlier this is a breaking change, because the below code would stop to work in browser builds:

const pathToRegexp = require('path-to-regexp')
pathToRegexp.compile('xxx')

So i did the minimal thing that is just add the default export and not the named export.

But again, i can change to make compatible with all

wesleytodd · 2019-06-24T18:53:56Z

It is not a breaking change if the new entry point file never existed before. Don't change the existing files at all and just change your transform step to transform the existing file to proper export statements.

This conversation has gone further than I hoped, so to avoid spending more time on this I will just say we should wait on an opinion from @blakeembrey who manages this package.

blikblum · 2019-06-24T19:00:31Z

It is not a breaking change if the new entry point file never existed before.

Nope, because webpack and rollup (or any tool that understand package.json module field) will start to load the index.esm.js (or index.mjs) instead of index.js automatically

Don't change the existing files at all and just change your transform step to transform the existing file to proper export statements.

The behavior of current file is not changed at all since the assignment is still being done to the same instance - pathToRegexp (the changes are just to make code clear but is not necessary at all)

wesleytodd · 2019-06-24T20:08:08Z

will start to load the index.esm.js (or index.mjs) instead of index.js automatically

Good call, yep it is breaking. In that case, I stand by my opinion that this change not be made at this time.

blikblum · 2019-06-24T20:20:54Z

Added named exports and renamed index.esm.js to index.mjs as requested

Node users are not affected at all, since no "type": "module" entry is added. This can / should be done separately since node support is in flux

By using both default and named exports ensures compatibility with all code that uses the library currently through a bundler like webpak or rollup.

In the future, in a major version may (optionally) change to use only default or only named exports

blakeembrey · 2019-07-01T01:32:35Z

Hi @blikblum, does every known CommonJS bundler support export default and module.exports interop like this PR is suggesting? IIRC even TypeScript doesn't understand this without a flag enabled, which would mean imports between JS, ESM and TypeScript definitions will be inconsistent with this change. I'd prefer we change everything in one go, to something that every current loader will understand.

blakeembrey · 2019-07-01T01:34:59Z

tools/create-esm.js

+
+const cjsSource = fs.readFileSync('index.js', { encoding: 'utf8' })
+
+const esmSource = cjsSource.replace(exportRegex, 'export default pathToRegexp\nexport { parse, compile, tokensToFunction, tokensToRegExp }\n')


Unfortunately I won't be able to accept this line as is. It actually deviates from existing usages meaning this is a breaking change (e.g. anyone using TypeScript or Webpack could be doing import * as pathToRegexp and using it as a function).

blikblum · 2019-07-01T09:59:49Z

I'd prefer we change everything in one go, to something that every current loader will understand.

Only the bundlers that understand will load the file pointed by "module" field (in this case index.mjs) the others will still load the file pointed by "main" (index.js)

It actually deviates from existing usages meaning this is a breaking change (e.g. anyone using TypeScript or Webpack could be doing import * as pathToRegexp and using it as a function).

No. Is there to keep current usages avoiding breaking changes, otherwise this would not work: import pathtoregexp from 'path-to-regexp'

The current setup covers (in bundlers):

import pathtoregexp from 'path-to-regexp' // import the default import
import * as MyPathToRegexp from 'path-to-regexp' // import the named imports
import { compile } from 'path-to-regexp' // import the named imports

const pathToRegexp = require('path-to-regexp') // import the default import
const {compile} = require('path-to-regexp') // import the default import

blakeembrey · 2019-07-01T19:24:20Z

No, it doesn’t cover all the usages. In TypeScript and probably Babel you can still use the asterisk import as a function. This code would not work for that, making this a breaking change.

blikblum · 2019-07-01T19:52:11Z

. In typescript and probably Babel you can still use the asterisk import as a function

At least babel/parcel you cant: https://codesandbox.io/s/3068znm201

Gives "pathToRegexp is not a function"

And babel/webpack also not: https://codesandbox.io/s/serene-meitner-0309o

blikblum · 2019-07-01T19:55:23Z

And typescript: https://codesandbox.io/s/cool-hofstadter-7c0j6

Maybe in a different ts config?

blakeembrey · 2019-07-01T21:03:55Z

@blikblum Remove esModuleInterop, it's the default configuration.

blikblum · 2019-07-02T12:53:26Z

@blakeembrey

Remove esModuleInterop, it's the default configuration.

If using a package that ships only ES module package like path-to-regexp-es it breaks

But using a package that distributes both commonjs pointed by "main" and es module pointed by "module", like this PR, it works. See yourself:
https://codesandbox.io/s/crazy-jackson-nbxgu

path-to-regexp-es-cjs is a package i published using the branch from this PR

abdonrd · 2019-08-13T10:51:00Z

Any news here?

And by the way, this could perhaps simplify: https://github.com/pikapkg/pack

frenzzy · 2019-09-25T20:15:05Z

Hi! I prepared a PR with alternative solution here: #197

In my opinion the main reason why we need ESM version is to enable tree shaking which helps to make client-side bundles smaller.

blakeembrey · 2019-11-11T06:16:05Z

Added support with 4124c3c.

abdonrd · 2019-11-11T08:15:35Z

Hi @blakeembrey!

Did you publish it to npm? I can't find it. Example with unpkg:
https://unpkg.com/browse/[email protected]/

blakeembrey · 2019-11-11T09:45:56Z

@abdonrd No, it is not released. You can see the commits and release history. Please be patient.

abdonrd · 2019-11-11T09:53:23Z

@blakeembrey ok, thanks Blake!

Add build step to create ES module version

bb3519b

blikblum mentioned this pull request Jun 24, 2019

Please provide es module distributable #174

Closed

paulmelnikow reviewed Jun 24, 2019

View reviewed changes

Rename index.esm.js to index.mjs

84f2009

Add named exports to esm build

a913bb8

blakeembrey reviewed Jul 1, 2019

View reviewed changes

frenzzy mentioned this pull request Oct 27, 2019

Add TypeScript and ES2015 modules #203

Closed

blakeembrey closed this Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ES module version #190

Add ES module version #190

blikblum commented Jun 24, 2019

coveralls commented Jun 24, 2019 •

edited

Loading

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019

paulmelnikow Jun 24, 2019

blikblum Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019 •

edited

Loading

blikblum commented Jun 24, 2019 •

edited

Loading

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

blakeembrey commented Jul 1, 2019

blakeembrey Jul 1, 2019

blikblum commented Jul 1, 2019

blakeembrey commented Jul 1, 2019 •

edited

Loading

blikblum commented Jul 1, 2019

blikblum commented Jul 1, 2019

blakeembrey commented Jul 1, 2019

blikblum commented Jul 2, 2019

abdonrd commented Aug 13, 2019

frenzzy commented Sep 25, 2019

blakeembrey commented Nov 11, 2019

abdonrd commented Nov 11, 2019

blakeembrey commented Nov 11, 2019

abdonrd commented Nov 11, 2019


		const cjsSource = fs.readFileSync('index.js', { encoding: 'utf8' })

		const esmSource = cjsSource.replace(exportRegex, 'export default pathToRegexp\nexport { parse, compile, tokensToFunction, tokensToRegExp }\n')

Add ES module version #190

Add ES module version #190

Conversation

blikblum commented Jun 24, 2019

coveralls commented Jun 24, 2019 • edited Loading

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019

paulmelnikow Jun 24, 2019

Choose a reason for hiding this comment

blikblum Jun 24, 2019

Choose a reason for hiding this comment

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019 • edited Loading

blikblum commented Jun 24, 2019 • edited Loading

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

wesleytodd commented Jun 24, 2019

blikblum commented Jun 24, 2019

blakeembrey commented Jul 1, 2019

blakeembrey Jul 1, 2019

Choose a reason for hiding this comment

blikblum commented Jul 1, 2019

blakeembrey commented Jul 1, 2019 • edited Loading

blikblum commented Jul 1, 2019

blikblum commented Jul 1, 2019

blakeembrey commented Jul 1, 2019

blikblum commented Jul 2, 2019

abdonrd commented Aug 13, 2019

frenzzy commented Sep 25, 2019

blakeembrey commented Nov 11, 2019

abdonrd commented Nov 11, 2019

blakeembrey commented Nov 11, 2019

abdonrd commented Nov 11, 2019

coveralls commented Jun 24, 2019 •

edited

Loading

wesleytodd commented Jun 24, 2019 •

edited

Loading

blikblum commented Jun 24, 2019 •

edited

Loading

blakeembrey commented Jul 1, 2019 •

edited

Loading