uuidjs / uuid

Generate RFC-compliant UUIDs in JavaScript
MIT License
14.62k stars 902 forks source link

ECMAScript Module / Browser Support #330

Closed ctavan closed 4 years ago

ctavan commented 5 years ago

I've spent some time continuing the work started in #317

I have the following high-level objectives:

So far I have tried to build a hybrid npm package that contains both, a CommonJS build and an esm build. I followed the pattern, that https://date-fns.org/v2.2.1/docs/ECMAScript-Modules uses.

This works fine for CommonJS-usage in Node.js and esm-usage with bundlers like webpack for the browser.

However it does not play nice with pure esm usage:

At this point I'm not 100% certain how to move forward and any feedback would be highly appreciated @broofa @defunctzombie.

ctavan commented 5 years ago

@broofa I have worked a bit more on #331 and I believe it is in a shape now that could be reviewed.

So what we could get with the current code is the following (almost as already described above):

  1. CommonJS build for Node.js just like right now.
  2. In addition a true esm build for the browser for use in webpack/rollup which supports treeshaking, this makes use of the "module": "esm-browser/index.js", property in package.json.
  3. As mentioned above, we can currently not support a true esm build for node (with type: module set in package.json) in the same npm package, see the announcement of node modules. Using the .mjs extension it is however theoretically possible to deep-import an esm build in node as well, see this example. An alternative would be to publish a separate uuid-es-node package.
  4. Direct import as an esm in the browser (without bundler) technically works as well, see this example.

Regarding 1.

Regarding 2.

Regarding 3.

Regarding 4.

I personally believe that we definitely want 1. & 2. and could wait a bit to see if anybody is asking for 3. & 4.

Based on your feedback I will make sure the README reflects all changes and will polish up the code.

Would be great to get this out soon! I think this should be released with a new major version number and we should also take this opportunity to finally resolve #173.

broofa commented 5 years ago

Hi Christoph, this is great work! More comments to come here and in the PR, but given the wholesale rewrite nature of this change, I wanted to make sure we acknowledge this for what it is: A de-facto changing of the guard.

Moving forward, I expect you'd be doing most of the heavy lifting maintaining this. I'll still help out as time allows and review/approve PRs as needed, but I'll probably look to you to lead when it comes to dealing with issues related to the new code.

Once this goes out, we should make you an Admin here, and an Owner in NPM.

Cc: @defunctzombie - You okay with this?

defunctzombie commented 5 years ago

My personal feeling is to favor a typescript port rather than mjs - but I also am not invested time wise into this project so if you are ok with this and phasing it over that's fine by me.

ctavan commented 5 years ago

@broofa thanks for being so open to the changes I have been suggesting! I do have enough spare time in the foreseeable future to maintain this repository and I'm happy to do so! However, while I'm willing to do the heavy lifting, I will be relying on your feedback in order to find suitable design decisions.

@defunctzombie thanks for your support! I'd be curious in your thoughts on the benefits of a TypeScript. I personally do not have a lot of hands on experience with TypeScript so I would be happy to learn from you. I have created a separate issue (#334) for this where you may want to elaborate a bit more on the benefits of a TypeScript rewrite?

broofa commented 5 years ago

Should we rename the imports from v4 to v4uuid?

If we do anything with the API, I would suggest we move in whatever direction https://github.com/tc39/proposal-uuid is going. So... start by having v4 as the default export, I guess?

There's surprisingly little guidance to be found in other languages, btw. Python has "uuid1".."uuid5" (which I dislike because it's ambiguous if it's a different version of uuid library or uuid format). Java and C# only support v4 uuids, so no help there. Golang is a bit of a clusterfuck.

If I had to make an argument for the current scheme, it's that it's concise and backward-compatible. It's also not that bad. If someone wants to be as explicit as "v4uuid", thenimport uuid from 'uuid' allows for uuid.v4(), which isn't terrible. Or they can import {v4 as uuid} from 'uuid'. ("uuid" instead of "v4uuid" because use cases where there's more than one version of uuid needed are exceedingly rare.)

How do you feel about exporting a separate package?

This isn't ideal, but it may be the least-obnoxious approach for the time being. What are you thinking? Have a script that builds and publishes ESM-specific packages? I'd be okay with that.

ctavan commented 5 years ago

How do you feel about exporting a separate package?

This isn't ideal, but it may be the least-obnoxious approach for the time being. What are you thinking? Have a script that builds and publishes ESM-specific packages? I'd be okay with that.

Yes, I was thinking about having a script that automatically publishes an esm-specific package for node (similary to how lodash is doing it). However given that this really still seems to be experimental in node and that it's still unclear, if/how node will support npm packages that contain CommonJS and ESM, we might as well just wait a bit for the fog to vanish before releasing yet another package to npm that might be difficult to get rid of later. Given the experimental nature I doubt that esm in node is commonly used yet (and for the brave who want to try we can even still provide a deep-importable build).

I believe the canonical use cases for node (=CommonJS) and the browser (=ESM for webpack/rollup) are covered with the current state of the code, so maybe let's postpone the advanced ESM stuff a bit?

ctavan commented 5 years ago

Will also stick with the current API surface and rather focus on making the documentation even more helpful (e.g. emphasizing v4 in the Quickstart).

ctavan commented 5 years ago

After digging deep into the current state of es modules in node I finally found https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md#phase-4-further-improvements-after-unflagging which states:

  • Dual CommonJS/ESM packages: Either support packages with both CommonJS and ESM sources that can be used in either environment; or decide to specifically not support dual CommonJS/ESM packages.
    • Status quo (at time of possible unflagging): "main" points to exactly one file, and all file extensions are mandatory (by default), so there is no possibility of an import specifier pointing to different files in ESM versus CommonJS. Recommended practice for dual packages is to have "main" point to the CommonJS entry point and have users use a deep import, e.g. /module.mjs, to access ESM entry point.

I will therefore, for now, not include any support form ESM in node and only keep the ESM browser-build that bundlers will pick up given the living standard of the module: "" package.json property.

ctavan commented 5 years ago

Tracking progress for ESM in Node.js in #342

ctavan commented 4 years ago

This is in the next branch since #337.