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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ef04be
Support --require of ESM; closes #4281
JacobLey May 27, 2020
f28fd67
Conditionally generate url for import
JacobLey May 28, 2020
e9834fd
Add tests for --require ESM
JacobLey May 28, 2020
ebc2a07
Revert change to eslintrc, use mocha to pass experimental flag
JacobLey May 28, 2020
c411c30
Replace type() -> typeof
JacobLey May 28, 2020
a6029b9
Remove doc limitation for --require ESM
JacobLey May 29, 2020
cc401ce
Add note to --require docs about ESM support
JacobLey May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ overrides:
browser: false
- files:
- test/**/*.{js,mjs}
parserOptions:
ecmaVersion: 2018
env:
mocha: true
globals:
Expand Down
5 changes: 4 additions & 1 deletion lib/cli/run-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ exports.handleRequires = async (requires = []) => {
debug('resolved required file %s to %s', mod, modpath);
}
const requiredModule = await requireOrImport(modpath);
if (type(requiredModule) === 'object' && requiredModule.mochaHooks) {
if (
['object', 'module'].includes(type(requiredModule)) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

node -e "import('./test/integration/fixtures/options/require/root-hook-defs-esm.fixture.mjs').then(mod => console.log(Object.prototype.toString.call(mod)))"
-> [object Module]

Piped through the type function: https://github.com/mochajs/mocha/blob/master/lib/utils.js#L216
Returns module.

So added handling for that "module" case (which seems reasonable considering that is what we would expect from ESM... but for the sake of discussing alternatives, could add some handling to type() to map module -> object

Copy link
Contributor

Choose a reason for hiding this comment

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

no, this seems correct. but, we should also add a test in test/node-unit/utils.spec.js to cover the behavior. you don't need to do this unless you want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can change it to return object if there are unforeseen consequences. what does typeof requiredModule return?

Copy link
Contributor Author

@JacobLey JacobLey May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

node -e "import('./test/integration/fixtures/options/require/root-hook-defs-esm.fixture.mjs').then(mod => console.log(typeof mod))" -> object

I'll let you interpret that... since there must be some reason not to use typeof directly.

add a test to cover the behavior

I can add that as well, so long as we are fine with Modules returning "module"

Copy link
Contributor

Choose a reason for hiding this comment

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

there's no real great reason to use type() here other than trying to maintain some consistency. we should use typeof and not worry about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ref: #4306

requiredModule.mochaHooks
) {
const mochaHooksType = type(requiredModule.mochaHooks);
if (/function$/.test(mochaHooksType) || mochaHooksType === 'object') {
debug('found root hooks in required file %s', mod);
Expand Down
Empty file added test/integration/fixtures/esm/require.mjs
View file
Open in desktop
Empty file.
1 change: 1 addition & 0 deletions test/integration/fixtures/options/require/esm/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{ "type": "module" }
8 changes: 8 additions & 0 deletions test/integration/fixtures/options/require/esm/root-hook-defs-esm.fixture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export const mochaHooks = () => ({
Copy link
Contributor

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');
},
});
8 changes: 8 additions & 0 deletions test/integration/fixtures/options/require/root-hook-defs-esm.fixture.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export const mochaHooks = {
beforeAll() {
console.log('mjs beforeAll');
},
afterAll() {
console.log('mjs afterAll');
},
};
46 changes: 46 additions & 0 deletions test/integration/options/require.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,51 @@ 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
Contributor

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'
)
],
{
env: {
...process.env,
Copy link
Contributor Author

@JacobLey JacobLey May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is what I updated the parser options (parserOptions: 2018) in eslintrc for.

Considering mocha is documented with:

As of v7.0.0, Mocha requires Node.js v8.0.0 or newer.

This seems safe enough for tests at least.

If there is an easier way to pass --experimental-modules to the mocha (instead of calling node directly) happy to use that instead...

Copy link
Contributor

@boneskull boneskull May 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I apologize for the state of things (we're working on it; see #4293), but...

I wanted to get a subset of the integration tests (test/integration/) running in a browser, but haven't completed this work. Stuff in test/node-unit/ (and a few other places) is allowed to use ES2018 syntax, but basically everything else under test/ is limited to ES5. It's probably OK to allow ES2018 in test/integration/, if we expect #4293 to get merged in the near future, since we'll be able to transpile everything down to where it needs to be. I don't envision browser-based integration tests landing before then, so IMO it's fine. Others may disagree (@mochajs/core ?)

However, stuff in test/unit/ and test/browser cannot, at this time, be ES2018, because these tests run in IE11. So, please revert the change to .eslintrc.yml, then add 'test/integration/**/*.js' to the list of overrides which already use parserOptions: 2018 (L39 or so).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here, you're running bin/mocha (which is what invokeMocha does), but using the NODE_OPTIONS environment variable to set --experimental-modules. You don't need to do this; just pass --experimental-modules in your array of args. bin/mocha knows what to do with it.

e.g.

invokeMocha(['path/to/fixture', '--some-other-flag'].concat(+process.versions.node.split('.')[0] >= 13 ? [] : ['--experimental-modules'], function() {})

I realize this means you can just revert the change to .eslintrc.yml entirely, which is maybe what should happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted change to eslintrc

Didn't realize I could pass the flag directly though mocha! Since I don't need any lint changes, I don't think this is the place to reorganize what is allowed where.

NODE_OPTIONS:
+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