ESM support #113

Avaq · 2019-03-21T14:39:49Z

Motivation

To close #99. I started this a long time ago, but never finished it. Today I spent a few minutes adding the finishing touches, although completely untested and written by hand.

Changes

Adds a new choice to the module option: esm. When running doctest with Node version 9 or up available, setting module to esm allows for the documentation comments to be embedded in ECMAScript modules, and the usage of import to load dependencies.

The approach included in this commit has the following benefits and consequences:

The CLI transparently switches between ESM and non-ESM enabled based on the Node version running it. This means the CLI is fully backwards compatible.
The programmatic version also transparently switches between ESM and non-ESM depending on whether it's loaded via import or via require.
The programmatic version has a breaking change, in that its primary function returns a Promise now.

TODO

Working proof of concept
Find a linting solution
Update and extend the unit tests
Polish and maximize code reuse

lib/doctest.js

danse

It's great work, thanks for submitting! I wouldn't know how to assess the impact on the users although supporting more idioms is definitely welcome!

lib/doctest.js

Avaq · 2019-03-25T09:06:35Z

Since import() is not available in cjs modules, and require() is not available in esm modules, it takes quite a bit of hackery getting this working, especially whilst keeping support for older Node versions (it would be only newer node versions that support esm).

The only way to interop between cjs and esm is to use esm to import a cjs module directly. So the idea is to create an esm module that dispatches to the cjs module for all functionality except esm support. I've started work on it, but it's slow going.

lib/command.js

.eslintignore

Avaq · 2019-03-31T16:41:17Z

.eslintrc.json

@@ -2,6 +2,9 @@
  "root": true,
  "extends": ["./node_modules/sanctuary-style/eslint-es3.json"],
  "env": {"node": true},
+  "globals": {
+    "Promise": "readonly"


I'm using native Promises in the cjs version to align its output type with the mjs version. If you don't align these types, tooling tends to get confused, not to mention people.

bin/doctest

lib/command.mjs

Avaq · 2019-03-31T16:43:14Z

lib/common.js

+//  unlines :: Array String -> String
+exports.unlines = function(lines) {
+  return lines.reduce (function(s, line) { return s + line + '\n'; }, '');
+};


I realize that common.js might be a badly chosen name, seeing as it already means something within the very domain we're working with. I intended it to have "common functionality", which is where it gets the name.

lib/doctest.js

lib/command.js

test/index.js

bin/doctest

lib/command.js

Avaq · 2019-04-01T09:53:41Z

.eslintrc.json

+    {
+      "files": ["*.mjs"],
+      "env": {"es6": true},
+      "parser": "babel-eslint"


We need a special parser for our files, because import() isn't an officially supported part of the language yet. The work around here is that we "pretend" to have written our source for Babel, while in reality we've written it for node --experimental-modules.

Avaq · 2019-04-01T11:41:02Z

test/index.js

  try {
-    stdout = execSync (command, {encoding: 'utf8', stdio: 'pipe'});
+    stdout = execSync (
+      command + (noWarnings ? ' --nodejs --no-warnings' : ''),


I'm passing --no-warnings to the Node executable via the --nodejs flag. This is to suppress the warning that Node gives when using --experimental-modules, which causes stderr assertions to fail.

Avaq · 2019-04-01T11:43:02Z

test/index.js

+  status: 1,
+  stdout: '',
+  stderr: unlines ([
+    'error: No files for doctesting provided'


Because the errors are now generated by the doctest function, giving no files would mean seeing no errors. This error ensures that you'll still see what went wrong when no files are provided.

test/index.js

Avaq · 2019-04-01T12:16:34Z

@davidchambers @danse I've marked the PR as ready for review. Since opening this PR, I've had to change the approach completely, but things appear to be working great now! I've already discussed some of the changes with David in Gitter. I'll paste some here:

bin/doctest now always spawns a sub-process with the --experimental-modules flag, if Node has version 9 or up. Depending on the presence or absence of that flag, Node automatically has a preference for .mjs or .js files.
- If --experimental-modules is absent, the sub-command is resolved to lib/doctest.js, and the old process continues from there.
- If --experimental-modules is present, the sub-command is resolved to lib/doctest.mjs, which can make a choice based on the --module setting.
lib/doctest.js has remained nearly unchanged, except that it returns a Promise now, and emits a new error message when --module is esm.
lib/doctest.mjs contains the esm parser and evaluator, and delegates work to lib/doctest.js in case --module is not esm.

This approach means that:

The CLI transparently switches between ESM and non-ESM enabled version based on the Node version running it. This means the CLI is fully backwards compatible.
The programmatic version also transparently switches between ESM and non-ESM depending on whether it's loaded via import or via require.
The programmatic version has a breaking change, in that it returns a Promise now.

davidchambers · 2019-04-01T13:43:15Z

Fantastic work, Aldwin!

lib/common.js

lib/doctest.js

lib/doctest.mjs

package.json

test/esm/incorrect.mjs

test/esm/index.mjs

test/index.js

lib/doctest.mjs

davidchambers · 2019-04-02T10:07:16Z

README.md

@@ -48,14 +48,13 @@ The exit code is 0 if all tests pass, 1 otherwise.

 ### AMD and CommonJS modules


We should generalize this heading.

bin/doctest

lib/command.js

lib/common.js

lib/doctest.js

bin/doctest

test/index.js

README.md

Avaq · 2019-04-02T10:24:52Z

(I stopped doing amendments once I saw you push a commit.)

README.md

bin/doctest

Avaq · 2019-04-03T09:12:34Z

@danse @davidchambers I'm very happy with this in its current form. Let me know if I can squash! :D

lib/command.js

davidchambers · 2019-04-03T10:03:31Z

Looking good! Please squash. :)

This commit adds a new choice to the 'module' option: 'esm'. When running doctest with Node version 9 or up available, setting 'module' to 'esm' allows for the documentation comments to be embedded in ECMAScript modules, and use 'import' to load dependencies. The approach included in this commit has the following consequences: 1. The CLI transparently switches between ESM and non-ESM enabled based on the Node version running it. This means the CLI is fully backwards compatible. 2. The programmatic version also transparently switches between ESM and non-ESM depending on whether it's loaded via import or via require. 3. The programmatic version has a breaking change, in that its primary function returns a Promise now. Co-Authored-By: David Chambers <dc@davidchambers.me>

Avaq · 2019-04-03T10:34:28Z

⚠️ Don't forget that this is a breaking change to the programmatic API.

davidchambers

🌳

danse · 2019-04-05T12:48:09Z

thanks @Avaq for your contribution and great work you two, the project keeps developing and the code keeps getting cleaner :)

Avaq commented Mar 21, 2019

View reviewed changes

lib/doctest.js Outdated Show resolved Hide resolved

Avaq commented Mar 21, 2019

View reviewed changes

lib/doctest.js Outdated Show resolved Hide resolved

Avaq commented Mar 21, 2019

View reviewed changes

lib/doctest.js Outdated Show resolved Hide resolved

danse reviewed Mar 21, 2019

View reviewed changes

lib/doctest.js Outdated Show resolved Hide resolved

Avaq mentioned this pull request Mar 29, 2019

Add everything fluture-js/fluture-node#1

Merged

3 tasks

Avaq force-pushed the avaq/esm branch from 0d38ede to 8e3a95e Compare March 31, 2019 16:06

davidchambers reviewed Mar 31, 2019

View reviewed changes

lib/command.js Outdated Show resolved Hide resolved

Avaq force-pushed the avaq/esm branch from 8e3a95e to 1a8f8d0 Compare March 31, 2019 16:10

Avaq commented Mar 31, 2019

View reviewed changes

davidchambers reviewed Mar 31, 2019

View reviewed changes

bin/doctest Outdated Show resolved Hide resolved

davidchambers reviewed Mar 31, 2019

View reviewed changes

bin/doctest Outdated Show resolved Hide resolved

Avaq force-pushed the avaq/esm branch from 583c444 to cb5d661 Compare March 31, 2019 17:37

davidchambers reviewed Mar 31, 2019

View reviewed changes

bin/doctest Outdated Show resolved Hide resolved

Avaq commented Apr 1, 2019

View reviewed changes

lib/command.js Show resolved Hide resolved

Avaq commented Apr 1, 2019

View reviewed changes

Avaq marked this pull request as ready for review April 1, 2019 11:56

Avaq commented Apr 1, 2019

View reviewed changes

test/index.js Show resolved Hide resolved

Avaq force-pushed the avaq/esm branch from 05b0acb to 440668b Compare April 1, 2019 11:58

davidchambers reviewed Apr 1, 2019

View reviewed changes

lib/common.js Outdated Show resolved Hide resolved

davidchambers reviewed Apr 1, 2019

View reviewed changes

lib/doctest.js Show resolved Hide resolved

davidchambers reviewed Apr 1, 2019

View reviewed changes

lib/doctest.mjs Outdated Show resolved Hide resolved

davidchambers reviewed Apr 1, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

davidchambers reviewed Apr 1, 2019

View reviewed changes

test/esm/incorrect.mjs Outdated Show resolved Hide resolved

davidchambers reviewed Apr 1, 2019

View reviewed changes

test/esm/index.mjs Outdated Show resolved Hide resolved

davidchambers reviewed Apr 1, 2019

View reviewed changes

test/index.js Outdated Show resolved Hide resolved