ES-Module support #331

ctavan · 2019-09-23T22:16:11Z

Discussion: #330

Node.js 6 has reached end-of-life at 2019-04-30 so there's little use of supporting this for future releases of uuid, see https://github.com/nodejs/Release#end-of-life-releases

BREAKING CHANGE: CommonJS what?

ctavan · 2019-10-13T12:12:47Z

@43081j @DarylThayil since you have commented on #317: Would you mind testing out my development branch from this PR by installing the following package directly from github:

  "dependencies": {
    "uuid": "github:ctavan/uuid-esm#master"
  }

You can have a look at some examples of how this development version can be used with esmodules in https://github.com/kelektiv/node-uuid/tree/c1307d3fde47d2191bee36d5ea2f42310c63fef8/examples

Would be curious about your feedback!

broofa

Should the esm-* directories include CLI files (uuid-bin.* and bin dir)? esm-browser/bin/uuid doesn't really make sense.

Naming nit: Having a "src/lib" directory is a little weird. "src" and "lib" are typically peer directories. Not sure what this should get renamed to but... something. Maybe "src/common"? Or maybe just pull the "lib" files up into "src" at this point? The main reason those were in "lib" was so we could put just the top-tier dependencies (v1/v3/v4/v5.js) at the top level. But if all the JS is getting pushed down into "src", then just having a single dir is probably fine.

Not approving yet, as I think we need to resolve how we want to publish browser-esm .v node-esm .v node-commonjs versions of this library, and I need to actually do my day job right now. Just want to push these comments up, otherwise I'll just sit on this indefinitely. 😛

broofa · 2019-10-14T13:59:19Z

examples/browser-esmodules/example.html

@@ -0,0 +1,3 @@
+<!DOCTYPE html>
+<title>UUID esmodule example</title>
+<script type="module" src="example.js"></script>


'Should probably have some verbiage in the BODY to inform readers where the actual code output is. E.g. "Please open Developer Console to view output"... something like that.

Will add explanation to the files.

How do you feel about adding these examples at all? Helpful or not?

broofa · 2019-10-14T13:59:49Z

examples/browser-esmodules/example.js

+
+console.log('uuidv1: ', uuidv1());
+
+


Style nit: Avoid multiple newlines

Consider adding no-multiple-empty-lines to eslintrc.json?

I would even consider to add prettier to get rid of any formatting discussion. However since this PR is already getting really big I wanted to push these kinds of tooling changes and refactorings to a different PR. Fine for you?

broofa · 2019-10-14T16:50:24Z

scripts/package.sh

+rm -rf "$DIR"
+mkdir -p "$DIR"
+
+# Traspile CommonJS versions of files


Suggested change

# Traspile CommonJS versions of files

# Transpile CommonJS versions of files

broofa · 2019-10-14T16:50:39Z

scripts/package.sh

+# Traspile CommonJS versions of files
+env TARGET='commonjs' babel src --source-root src --out-dir "$DIR" --copy-files --quiet
+
+# Traspile ESM versions of files for node


Suggested change

# Traspile ESM versions of files for node

# Transpile ESM versions of files for node

broofa · 2019-10-14T16:52:49Z

scripts/package.sh

+for FILE in "$DIR"/esm-browser/lib/*-browser.js
+do


for FILE in "$DIR"/esm-browser/lib/*-browser.js; do

Style nit: Prefer one line for declaring for-loops in Bash.

broofa · 2019-10-14T18:40:51Z

src/lib/v35.js

-  generateUUID.URL = '6ba7b811-9dad-11d1-80b4-00c04fd430c8';
+  // For CommonJS default export support
+  generateUUID.DNS = DNS;
+  generateUUID.URL = URL;


Maybe have the v3/v5.js files explicitly export DNS, URL from './lib/v35.js' rather than bodging it in here? (Would that work?)

I tried that but it does not seem to work.

I'm using https://www.npmjs.com/package/babel-plugin-add-module-exports to add module.exports to the transpiled CommonJS modules so people can continue to do

const uuidv4 = require('uuid/v4');

instead of having to do ugly workarounds like

const uuidv4 = require('uuid/v4').default;

or

const { default: uuidv4 } = require('uuid/v4');

As soon as there is more than export default … this module.exports approach will no longer work.

ctavan

@broofa thanks for the review, I will push changes as suggested.

Not approving yet, as I think we need to resolve how we want to publish browser-esm .v node-esm .v node-commonjs versions of this library, and I need to actually do my day job right now. Just want to push these comments up, otherwise I'll just sit on this indefinitely. 😛

As this PR has already grown almost too big to review I wanted to propose to create a new next branch from master and I would then post all my PRs against this next branch. That way we can take a bit more time and focus to flesh things out (also potentially publishing some prereleases) without putting master into a broken state.

I would then also make sure to keep PRs smaller so there's a more realistic chance for you to review.

Fine for you?

Mostly borrowed from https://date-fns.org/v2.2.1/docs/ECMAScript-Modules

ctavan · 2019-10-15T14:22:26Z

@broofa since I pushed so many fixups I decided to squash all my fixups together again.

Quick summary of my changes:

moved the ./lib code up one level
as per ECMAScript Module / Browser Support #330 went for CommonJS-build for node and esm build for browsers (webpack/rollup/etc. through module: and browser: properties in package.json
as discussed in ECMAScript Module / Browser Support #330 decided not to change the module API at this point
instead, removed the deprecation for the default import and instead adjusted the examples to use const uuid = require('uuid'); again -> browser builds where module size matters should now make use of esm!
made v4 the first quickstart example to work against the above-the-fold-v1-bias

Here's what the contents of the npm tarball would look like: https://github.com/ctavan/uuid-esm (you can also check out the updated README there)

Feature-wise I would love to merge this state into a next branch and make a uuid@4.0.0-alpha.1 prerelease.

I would then like to make some more tooling improvements (like publishing changelog to the Github releases page), maybe add browser tests (now that we have code specifically targeting the browser) and remove some outdated code like pre-node-4 Buffer code, but all of that in smaller, separate PRs.

broofa · 2019-10-16T17:15:25Z

This all sounds good. I like the idea of a next branch. Given that this is a top-20 NPM module, letting this simmer for a bit so we get more eyeballs on it is probably wise. It might be worth adding a header to the README on master informing people about this branch so they can weigh in.

I'd like to keep whatever we're doing here in sync with https://github.com/tc39/proposal-uuid, whatever that means. At a minimum, try to have some agreement on how the two projects interrelate would be prudent. E.g. Do we treat this module as the "experimental" sandbox for whatever API(s) might come later there? Dunno.

Note that your work here supersedes much of my es6-modules branch. We should probably just delete that branch to avoid confusion. My only hesitatio there is that the UUID class has some useful code in it for parsing, validation, and working with byte arrays. Part of me wants to say we should incorporate that, but I'm not sure I can make a good enough argument for that.

I'm traveling this week and next, but I'll try to review again ASAP.

ctavan · 2019-10-16T18:46:12Z

This all sounds good. I like the idea of a next branch. Given that this is a top-20 NPM module, letting this simmer for a bit so we get more eyeballs on it is probably wise. It might be worth adding a header to the README on master informing people about this branch so they can weigh in.

👍

I would need publish access for the npm module at some point though to publish 4.0.0-alpha prereleases.

Will make sure to update the master README.

I'd like to keep whatever we're doing here in sync with https://github.com/tc39/proposal-uuid, whatever that means. At a minimum, try to have some agreement on how the two projects interrelate would be prudent. E.g. Do we treat this module as the "experimental" sandbox for whatever API(s) might come later there? Dunno.

👍

However as discussed in #330 I will focus on ESModule interoperability and tooling changes for now and I will not introduce any API changes for the time being.

Should we decide to change the API of this module though, I totally agree that we should keep it in sync with https://github.com/tc39/proposal-uuid.

Note that your work here supersedes much of my es6-modules branch. We should probably just delete that branch to avoid confusion. My only hesitatio there is that the UUID class has some useful code in it for parsing, validation, and working with byte arrays. Part of me wants to say we should incorporate that, but I'm not sure I can make a good enough argument for that.

In fact I cherry-picked your commit from the other branch, I see value in leaving the other branch open for a while until we have made decisions around the API changes (which I intentionally left out here).

ctavan · 2019-10-16T18:52:15Z

Closing in favor of #337 337

ctavan and others added 11 commits September 23, 2019 23:58

docs: re-generate README

b9fd85b

test: remove tests on Node.js 6

edf430a

Node.js 6 has reached end-of-life at 2019-04-30 so there's little use of supporting this for future releases of uuid, see https://github.com/nodejs/Release#end-of-life-releases

test: let Travis run tests on Node.js 12

b82ed2d

chore: upgrade all devDependencies

bb2f9ae

chore: upgrade husky hooks config

3bf178d

chore: sort eslint envs alphabetically

2e9341f

style: convert tabs to spaces

0c4638b

chore: define global msCrypto to satisfy eslint

b22219f

test: remove unused randomFillSync variable

acd26d6

test: run eslint as part of the tests

d125e45

feat: convert to esmodules

9c249a2

BREAKING CHANGE: CommonJS what?

ctavan force-pushed the esm branch from 31ba7a2 to c1307d3 Compare October 13, 2019 12:10

ctavan mentioned this pull request Oct 13, 2019

ECMAScript Module / Browser Support #330

Closed

6 tasks

broofa reviewed Oct 14, 2019

View reviewed changes

ctavan commented Oct 14, 2019

View reviewed changes

ctavan mentioned this pull request Oct 14, 2019

Prepare for esm #335

Merged

ctavan added 2 commits October 15, 2019 16:01

feat: hybrid commonJS & es module build

d8756d7

Mostly borrowed from https://date-fns.org/v2.2.1/docs/ECMAScript-Modules

feat: add various es module and CommonJS examples

010a6af

ctavan force-pushed the esm branch from e4ce610 to f5a3763 Compare October 15, 2019 14:04

ctavan added 2 commits October 15, 2019 16:09

docs: document esm features

59d4d67

feat: ensure that docs are up-to-date in CI

bb5c2ab

ctavan force-pushed the esm branch from f5a3763 to bb5c2ab Compare October 15, 2019 14:09

ctavan changed the title ~~WIP ES-Module support~~ ES-Module support Oct 15, 2019

ctavan requested a review from broofa October 16, 2019 07:48

ctavan mentioned this pull request Oct 16, 2019

ES-Module support (next) #337

Merged

ctavan closed this Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES-Module support #331

ES-Module support #331

ctavan commented Sep 23, 2019

ctavan commented Oct 13, 2019

broofa left a comment

broofa Oct 14, 2019

ctavan Oct 14, 2019

broofa Oct 14, 2019

ctavan Oct 14, 2019

broofa Oct 14, 2019

broofa Oct 14, 2019

broofa Oct 14, 2019

broofa Oct 14, 2019 •

edited

Loading

ctavan Oct 14, 2019

ctavan left a comment

ctavan commented Oct 15, 2019

broofa commented Oct 16, 2019

ctavan commented Oct 16, 2019

ctavan commented Oct 16, 2019

	# Traspile CommonJS versions of files
	# Transpile CommonJS versions of files

	# Traspile ESM versions of files for node
	# Transpile ESM versions of files for node


		console.log('uuidv1: ', uuidv1());

ES-Module support #331

ES-Module support #331

Conversation

ctavan commented Sep 23, 2019

ctavan commented Oct 13, 2019

broofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

broofa Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctavan left a comment

Choose a reason for hiding this comment

ctavan commented Oct 15, 2019

broofa commented Oct 16, 2019

ctavan commented Oct 16, 2019

ctavan commented Oct 16, 2019

broofa Oct 14, 2019 •

edited

Loading