WIP: use dynamic import to load es module tests. by harrysarson · Pull Request #3253 · mochajs/mocha

harrysarson · 2018-02-26T23:28:05Z

Description of the Change

Uses the new (and as yet highly experimental) support in node for dynamically loading modules using const obj = await import('./file-name.mjs');.

I have added a new command line flag --es-modules which causes mocha to use import() instead of require() to load test files.

This allows tests to import obj from './somefile.mjs' without having to transpile tests before running them.

Obviously this only works in the very latest node version. This commit does not break mocha in node <= 8 unless the user passes the es-modules flag.

Currently running a script in node with --experimental-modules fails if the file does not have an extension which means that node --experimental-modules bin/_mocha files. From my understanding (see nodejs/node#18728) node v9.7 will be able to run scripts without the .js extension.

Alternate Designs

Current design works fine as long as users are happy to use a transpiler.

Why should this be in core?

Cannot be implemented outside of core.

Benefits

Tests can be written as es modules.

Possible Drawbacks

Current support for modules in node is experimental. It is likely that mocha will want to wait untill node starts properly supporting modules before merging this pr.

Applicable issues

#3006

Probably semver minor but could be major.

When the --es-modules flag is provided import() will be used to load all test files.

node 9.6 will not allow script files without .js extensions when --experimental-modules flag is passed. I think node 9.7 will allow this so this commit can be reverted then.

Node versions less than 9.6 do not support dynamic es imports. Currently using instanbul to calculate codecoverage calls some script without a .js file extension which causes the tests to fail. Hopefully in v9.7 this will be fixed.

Adds an integration test that uses cjs modules but is called with the --es-modules flag.

Dynamic imports are async and therefore errors need to be explicitly handled.

boneskull · 2018-03-01T01:12:50Z

This seems to break AppVeyor's build, but why?

boneskull

Thanks. I'm not entirely sure what you intended we do with this PR.

boneskull · 2018-03-01T01:14:07Z

  include:
    - node_js: '9'
-      env: TARGET=test.node COVERAGE=true
-    - node_js: '8'


we can't just remove all of these from the build matrix...

boneskull · 2018-03-01T01:14:51Z

 const path = require('path');
 const getOptions = require('./options');
-const args = [path.join(__dirname, '_mocha')];
+const args = [path.join(__dirname, '_mocha.js')];


why is this necessary?

boneskull · 2018-03-01T01:17:04Z

+ * Load registered files via es6 dynamic imports.
+ *
+ * Note: `eval` is used in this function or else mocha will not
+ * compile on older node versions.


I don't understand where this is supposed to work.

The new import() operator allows ECMAScript Modules to be loaded dynamically. The hope was that tests written as modules (i.e. tests including import obj from './script.js' statements) can be loaded into Mocha using dynamic imports without having to first use a transpiler to convert ECMAScript Modules into common js modules.

right, but I'm unclear on why we're removing old versions of Node.js from the matrix.

Current support for modules in node is experimental. It is likely that mocha will want to wait untill node starts properly supporting modules before merging this pr.

if we're going to break backwards compatibility, then it'd have to wait until the oldest version of Node.js that supports modules is no longer maintained. that'll be a few years. so, that's why I'm trying to understand what I'm supposed to do with this...

Sorry, I didn't mean to add that change to the pull request, I will revert it.

The idea was that mocha could add a new CLI flag which would cause mocha to use the new import() instead of require() to load test files.

This new CLI flag could only be used on the most recent versions of node but mocha would work normally if this flag was not set.

The tests fail because I added a test for the new flag and travis/appveyor runs this test on old versions of node, I didn't know how to work around that.

boneskull · 2018-03-01T01:17:28Z

+      file = path.resolve(file);
+      suite.emit('pre-require', global, file, self);
+      return eval('import(file)')
+        .then(function (module) {


probably shouldn't use module

This reverts commit 10736aa.

This reverts commit b28d2db.

demurgos · 2018-03-26T19:26:54Z

Why was this PR closed?

I am currently using Mocha together with @std/esm to test my .mjs files but this is suboptimal (edge cases around interop with cjs or code coverage). Now that dynamic imports are available it would be nice to be able to use ESM directly with Mocha.

boneskull · 2018-03-26T21:10:17Z

Until modules are unflagged and/or no longer considered "experimental", I'm concerned about adopting something too soon. Specifically, when there's agreement in core around interop between require() and import, filename extensions, etc.--then it's probably safe to move. Too early, and it'll be expensive to fix any mistakes we made by jumping on it too soon.

demurgos · 2018-03-27T15:30:14Z

Thanks for the reply: it's a fair reason.

I was worried that there was an issue with the PR preventing it from working. Still, it's good to know that it's a Node stability issue instead and that you are able to add support for it when possible.

harrysarson · 2018-03-27T16:37:32Z

I was a bit optimistic with this PR, definately need to wait until modules are finalised in node.

The other problem, which is why my work was so hacky, is that the dynamic import() loads modules asynchronously whereas mocha currently loads tests synchronously (using require()) so it is not as simple as just switching one for the other.

demurgos · 2018-06-25T14:20:17Z

Hi,
The modules discussion is still ongoing, but it seems that there's agreement that the imports will be async (ESM to ESM, ESM to CJS).

A possible way to prepare the support for ESM would be to force the current imports to be async by default: replace the resolution code from require("test.spec.js") to Promise.resolve(require("test.spec.js")).
Replacing the inner require by import would then be trivial.

Looking at the PR, it seems that most of the code is already async. I am not familiar enough with mocha's codebase so maybe my suggestion is not needed. I just hope that Mocha will be able to support ESM when available.

harrysarson · 2018-06-25T18:45:40Z

@demurgos from memory it should be fairly straightforward to make that change

harrysarson added 3 commits February 26, 2018 23:29

Adds flag --es-modules to dynamically import test files.

d46d557

When the --es-modules flag is provided import() will be used to load all test files.

Add .js extension to bin/_mocha to get tests passing

a7d5262

node 9.6 will not allow script files without .js extensions when --experimental-modules flag is passed. I think node 9.7 will allow this so this commit can be reverted then.

Only test node v9 and don't check coverage.

10736aa

Node versions less than 9.6 do not support dynamic es imports. Currently using instanbul to calculate codecoverage calls some script without a .js file extension which causes the tests to fail. Hopefully in v9.7 this will be fixed.

harrysarson force-pushed the es-modules branch from 856c911 to 10736aa Compare February 26, 2018 23:30

harrysarson added 2 commits February 26, 2018 23:41

Try running tests on appveyer with node v9

b28d2db

Test that cjs tests still work when using es-modules.

e1ad789

Adds an integration test that uses cjs modules but is called with the --es-modules flag.

harrysarson force-pushed the es-modules branch from 0108699 to e1ad789 Compare February 27, 2018 09:52

Catch and report errors thrown from top level of modules.

a16f8d0

Dynamic imports are async and therefore errors need to be explicitly handled.

boneskull added type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" labels Mar 1, 2018

boneskull suggested changes Mar 1, 2018

View reviewed changes

harrysarson added 2 commits March 7, 2018 12:12

Revert "Only test node v9 and don't check coverage."

a494924

This reverts commit 10736aa.

Revert "Try running tests on appveyer with node v9"

666a25f

This reverts commit b28d2db.

harrysarson closed this Mar 23, 2018

demurgos mentioned this pull request Jul 9, 2018

Support ES6 style tests without transpiler usage #3006

Closed

4 tasks

tbushman mentioned this pull request Jan 10, 2019

feat: add initial tests freeCodeCamp/tools-dashboard#60

Merged

Uh oh!

Uh oh!

Conversation

harrysarson commented Feb 26, 2018

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

Uh oh!

boneskull commented Mar 1, 2018

Uh oh!

boneskull left a comment

Choose a reason for hiding this comment

Uh oh!

boneskull Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

boneskull Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

boneskull Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

harrysarson Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

boneskull Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

harrysarson Mar 7, 2018

Choose a reason for hiding this comment

Uh oh!

boneskull Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

demurgos commented Mar 26, 2018

Uh oh!

boneskull commented Mar 26, 2018

Uh oh!

demurgos commented Mar 27, 2018

Uh oh!

harrysarson commented Mar 27, 2018

Uh oh!

demurgos commented Jun 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harrysarson commented Jun 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

demurgos commented Jun 25, 2018 •

edited

Loading