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

Support --require of ESM; closes #4281 #4304

Merged
merged 7 commits into from Jun 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/index.md
Expand Up @@ -1007,6 +1007,8 @@ Modules required in this manner are expected to do work synchronously; Mocha won

Note you cannot use `--require` to set a global `beforeEach()` hook, for example — use `--file` instead, which allows you to specify an explicit order in which test files are loaded.

> As of v7.3.0, Mocha supports `--require` for [NodeJS native ESM](#nodejs-native-esm-support). There is no separate `--import` flag.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that the version here may need changing


### `--sort, -S`

Sort test files (by absolute path) using [Array.prototype.sort][mdn-array-sort].
Expand Down Expand Up @@ -1450,7 +1452,6 @@ Node.JS native ESM support still has status: **Stability: 1 - Experimental**
- [Watch mode](#-watch-w) does not support ES Module test files
- [Custom reporters](#third-party-reporters) and [custom interfaces](#interfaces)
can only be CommonJS files
- [Required modules](#-require-module-r-module) can only be CommonJS files
- [Configuration file](#configuring-mocha-nodejs) can only be a CommonJS file (`.mocharc.js` or `.mocharc.cjs`)
- When using module-level mocks via libs like `proxyquire`, `rewiremock` or `rewire`, hold off on using ES modules for your test files
- Node.JS native ESM support does not work with [esm][npm-esm] module
Expand Down
19 changes: 13 additions & 6 deletions lib/cli/run-helpers.js
Expand Up @@ -15,6 +15,7 @@ const collectFiles = require('./collect-files');
const {type} = require('../utils');
const {format} = require('util');
const {createInvalidPluginError, createUnsupportedError} = require('../errors');
const {requireOrImport} = require('../esm-utils');

/**
* Exits Mocha when tests + code under test has finished execution (default)
Expand Down Expand Up @@ -81,16 +82,21 @@ exports.list = str =>
* @returns {Promise<MochaRootHookObject|MochaRootHookFunction>} Any root hooks
* @private
*/
exports.handleRequires = async (requires = []) =>
requires.reduce((acc, mod) => {
exports.handleRequires = async (requires = []) => {
const acc = [];
for (const mod of requires) {
let modpath = mod;
// this is relative to cwd
if (fs.existsSync(mod) || fs.existsSync(`${mod}.js`)) {
modpath = path.resolve(mod);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the file in question is a package name, path.resolve will fail [...]

Will this work for packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, fs.existsSync will return false for a package name, so it is not resolved at all (passed raw) to requireOrImport (e.g. requireOrImport('@babel/register')

This behavior is unchanged in this PR

debug('resolved required file %s to %s', mod, modpath);
}
const requiredModule = require(modpath);
if (type(requiredModule) === 'object' && requiredModule.mochaHooks) {
const requiredModule = await requireOrImport(modpath);
if (
requiredModule &&
typeof requiredModule === 'object' &&
requiredModule.mochaHooks
) {
const mochaHooksType = type(requiredModule.mochaHooks);
if (/function$/.test(mochaHooksType) || mochaHooksType === 'object') {
debug('found root hooks in required file %s', mod);
Expand All @@ -102,8 +108,9 @@ exports.handleRequires = async (requires = []) =>
}
}
debug('loaded required module "%s"', mod);
return acc;
}, []);
}
return acc;
};

/**
* Loads root hooks as exported via `mochaHooks` from required files.
Expand Down
17 changes: 11 additions & 6 deletions lib/esm-utils.js
@@ -1,11 +1,16 @@
const url = require('url');
const path = require('path');
const url = require('url');

const requireOrImport = async file => {
file = path.resolve(file);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved resolve() to loadFilesAsync so the file doesn't have to be a fully-resolved name, but can be a package from node_modules as well.

const formattedImport = async file => {
if (path.isAbsolute(file)) {
return import(url.pathToFileURL(file));
}
return import(file);
};

exports.requireOrImport = async file => {
if (path.extname(file) === '.mjs') {
return import(url.pathToFileURL(file));
return formattedImport(file);
}
// This is currently the only known way of figuring out whether a file is CJS or ESM.
// If Node.js or the community establish a better procedure for that, we can fix this code.
Expand All @@ -15,7 +20,7 @@ const requireOrImport = async file => {
return require(file);
} catch (err) {
if (err.code === 'ERR_REQUIRE_ESM') {
return import(url.pathToFileURL(file));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what url.pathToFileURL was achieving?
import() allows a plain string, and this way the required file can still be resolvable via node's logic (e.g. loading from node_modules)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it was required for Windows.
Gosh I hate windows

return formattedImport(file);
} else {
throw err;
}
Expand All @@ -25,7 +30,7 @@ const requireOrImport = async file => {
exports.loadFilesAsync = async (files, preLoadFunc, postLoadFunc) => {
for (const file of files) {
preLoadFunc(file);
const result = await requireOrImport(file);
const result = await exports.requireOrImport(path.resolve(file));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the file in question is a package name, path.resolve will fail (AFAIK with an exception). Why did you add a path.resolve here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. --require-d files can be module names, and so you didn't want requireOrImport, which is now used also in handleRequire, to resolve them to a path. So you moved it to loadFileAsync, where it is needed (for Windows, it seems...)

postLoadFunc(file, result);
}
};
1 change: 1 addition & 0 deletions test/integration/fixtures/options/require/esm/package.json
@@ -0,0 +1 @@
{ "type": "module" }
@@ -0,0 +1,8 @@
export const mochaHooks = () => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

beforeEach() {
console.log('esm beforeEach');
},
afterEach() {
console.log('esm afterEach');
},
});
@@ -0,0 +1,8 @@
export const mochaHooks = {
beforeAll() {
console.log('mjs beforeAll');
},
afterAll() {
console.log('mjs afterAll');
},
};
41 changes: 41 additions & 0 deletions test/integration/options/require.spec.js
@@ -1,6 +1,7 @@
'use strict';

var invokeMochaAsync = require('../helpers').invokeMochaAsync;
var utils = require('../../../lib/utils');

describe('--require', function() {
describe('when mocha run in serial mode', function() {
Expand Down Expand Up @@ -45,6 +46,46 @@ describe('--require', function() {
/beforeAll[\s\S]+?beforeAll array 1[\s\S]+?beforeAll array 2[\s\S]+?beforeEach[\s\S]+?beforeEach array 1[\s\S]+?beforeEach array 2[\s\S]+?afterEach[\s\S]+?afterEach array 1[\s\S]+?afterEach array 2[\s\S]+?afterAll[\s\S]+?afterAll array 1[\s\S]+?afterAll array 2/
);
});

describe('support ESM when style=module or .mjs extension', function() {
before(function() {
if (!utils.supportsEsModules()) this.skip();
});

it('should run root hooks when provided via mochaHooks', function() {
return expect(
invokeMochaAsync(
[
'--require=' +
require.resolve(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--requireworks with absolute path and module names. Does it work also with relative paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see where you commented on handleRequires, relative paths are transformed into absolute paths.

None of the tests use it though because the actual CWD isn't very clear in-test. But maybe it is possible to calculate a relative path in test and pass that through as well.

Again, that particular functionality is unchanged in this PR

// as object
'../fixtures/options/require/root-hook-defs-esm.fixture.mjs'
),
'--require=' +
require.resolve(
// as function
'../fixtures/options/require/esm/root-hook-defs-esm.fixture.js'
),
'--require=' +
require.resolve(
// mixed with commonjs
'../fixtures/options/require/root-hook-defs-a.fixture.js'
),
require.resolve(
'../fixtures/options/require/root-hook-test.fixture.js'
)
].concat(
+process.versions.node.split('.')[0] >= 13
? []
: '--experimental-modules'
)
)[1],
'when fulfilled',
'to contain output',
/mjs beforeAll[\s\S]+?beforeAll[\s\S]+?esm beforeEach[\s\S]+?beforeEach[\s\S]+?esm afterEach[\s\S]+?afterEach[\s\S]+?mjs afterAll[\s\S]+?afterAll/
);
});
});
});

describe('when mocha in parallel mode', function() {
Expand Down