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

ECMAScript Module / Browser Support #330

Closed
6 tasks done
ctavan opened this issue Sep 23, 2019 · 10 comments
Closed
6 tasks done

ECMAScript Module / Browser Support #330

ctavan opened this issue Sep 23, 2019 · 10 comments
Milestone

Comments

@ctavan
Copy link
Member

ctavan commented Sep 23, 2019

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

I have the following high-level objectives:

  • Modernize the code base
  • Provide a CommonJS build for node.js
  • Provide an ESM (ECMAScript Module) build for use in Browser-Bundlers like webpack, rollup etc.
  • Provide an ESM build for direct use in the browser
  • (Provide an ESM build for direct use in node 12 <-- Still unclear if/when Node.js will remove the experimental flag) Won't do as of Oct. 2019
  • Changelog

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:

  • For webpack and other bundlers we're saved by the "module": "esm/index.js" property in package.json, so far so good.
  • For Node.js, if we wanted true esm we'd have to publish the package with type: module in package.json and have the main property point directly to the esm build, the module property as mentioned above does not seem to work. This could be something like uuid-es similar to https://www.npmjs.com/package/lodash-es
  • For native esm usage in Browsers we run into yet another problem: We currently rely on the "browser" map in package.json to overwrite certain libraries in the browser context. This is again respected by webpack and other bundlers but obviously not by the browsers themselves. So here we'd maybe have to release another uuid-es-browser package with rng/sha1/md5 libraries swapped by the respective browser counterparts?
  • Still have to check on TypeScript…

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

@ctavan
Copy link
Member Author

ctavan commented Oct 13, 2019

@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.

  • Since bundlesize doesn't matter in node, should we remove the API deprecation and go back to using?
const uuid = require('uuid');
uuid.v4();

Regarding 2.

  • The ESM way of importing someting would now be
import {v4} from 'uuid';

Should we rename the imports from v4 to v4uuid? I personally find it rather confusing to have a function called v4() somewhere in application code (where there's no direct context of the import statement) which in fact generates a v4uuid.

Regarding 3.

  • How do you feel about exporting a separate package? Or maybe we just wait until the experimental flag vanishes and there will hopefully be a way to make an npm package work with both, CommonJS syntax and ESM.?

Regarding 4.

  • I have no clue if this is a use case at all or if most browser people wouldn't rather go for 2. anyways?!

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
Copy link
Member

broofa commented Oct 14, 2019

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
Copy link
Contributor

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
Copy link
Member Author

ctavan commented Oct 14, 2019

@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
Copy link
Member

broofa commented Oct 14, 2019

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
Copy link
Member Author

ctavan commented Oct 14, 2019

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
Copy link
Member Author

ctavan commented Oct 15, 2019

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
Copy link
Member Author

ctavan commented Oct 15, 2019

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 ctavan added this to the 4.0.0 milestone Oct 19, 2019
@ctavan
Copy link
Member Author

ctavan commented Oct 31, 2019

Tracking progress for ESM in Node.js in #342

@ctavan ctavan mentioned this issue Oct 31, 2019
10 tasks
@ctavan
Copy link
Member Author

ctavan commented Jan 20, 2020

This is in the next branch since #337.

@ctavan ctavan closed this as completed Jan 20, 2020
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

No branches or pull requests

3 participants