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

add zero-config support for loading ES modules in Node #3695

Closed
wants to merge 1 commit into from
Closed

add zero-config support for loading ES modules in Node #3695

wants to merge 1 commit into from

Conversation

jdalton
Copy link

@jdalton jdalton commented Jan 27, 2019

This PR adds zero-config support for loading ES modules in Node. This PR just replaces the entry require with esmRequire. Now --require modules, configs, and others files can have ESM syntax. The caveat is that .mjs files are not supported since under the hood mocha is still using require() to load files. See the following esm doc note:

💡 By Node’s rules, builtin require cannot sideload .mjs files. However, with esm, ES modules can be sideloaded as .js files or .mjs files may be loaded with dynamic import.

@boneskull Feel free to push tests to this PR.

@jsf-clabot
Copy link

jsf-clabot commented Jan 27, 2019

CLA assistant check
All committers have signed the CLA.

@boneskull
Copy link
Contributor

@jdalton I can push code to your branch? didn’t realize that was a thing.

@coveralls
Copy link

coveralls commented Jan 27, 2019

Coverage Status

Coverage decreased (-0.04%) to 90.692% when pulling c72ddc2 on jdalton:esm into 8de7fae on mochajs:next.

@jdalton
Copy link
Author

jdalton commented Jan 27, 2019

@boneskull Yep, it's a thing 🎉

Update:

PS: I think the one test fail in Node 6 on travis-ci might be a false fail.

@outsideris outsideris added the type: feature enhancement proposal label Jan 28, 2019
@boneskull
Copy link
Contributor

restarting the job... I haven't seen flake in those tests before.

@boneskull boneskull added the area: node.js command-line-or-Node.js-specific label Jan 28, 2019
@boneskull
Copy link
Contributor

this closes #3006

@jdalton
Copy link
Author

jdalton commented Jan 28, 2019

restarting the job... I haven't seen flake in those tests before.

Ah ok. Thank you. It looks like that test still fails so I'll dig in.

@boneskull
Copy link
Contributor

sorry about the rebase, I’m stepping on your toes here. let me know when you’re done

@jdalton
Copy link
Author

jdalton commented Jan 28, 2019

(no worries, rebase away)

Update:

I narrowed down the bug. It's an esm issue. I'll update tonight with a patch.

Update:

Turned out to be a timing issue.

@boneskull
Copy link
Contributor

So this will get esm in front of a LOT of people. I'm thinking it will probably hit some corner-cases. I'm not too worried about those getting fixed in a timely manner on the esm side, but it'd be nice to not block people who do encounter an issue.

To mitigate this, I'm thinking we should do one or more of:

  1. Provide an esm option which would default to true, allowing e.g., --no-esm to disable the integration.
  2. The above, but set this to false. There's a great argument for Mocha prioritizing a "real" testing environment (as opposed to Jest or other frameworks which may mock/stub/fake a bunch of stuff for whatever reason).
  3. Create an exception to our rule about exact production dependencies (that rule being "they should all be exact versions") using either a tilde or caret to accept semver-patch or semver-minor upgrades to esm as they are released. This would allow users encountering problems to easily get a fix in esm without Mocha needing to release.
  4. citgm-like tests against userland modules which monkey with require, Node.js' Module object, ES modules, stack traces, etc.
  5. new integration tests probing the behavior of the systems in the previous item

@jdalton Looking for some recommendations from you here... you will know better what we should be cautious about.

@boneskull
Copy link
Contributor

@mochajs/core What are your thoughts on releasing this in v6? v6.1? v7?

@jdalton
Copy link
Author

jdalton commented Jan 29, 2019

Thanks @boneskull!

You could check for the Node flag --experimental-modules and skip esm use in those situations as an intuitive toggle.

While the esm loader does mask some stack traces, I'm sure there's gaps. An easy esm option would be great!

An on-by-default might help folks realize they don't need to include Babel. Though I can understand if, when flighted, there are issues that it goes to an off-by-default toggle.

The esm field could also be implemented as a loader field so folks could make it something else too. Here is how AVA detects esm in their options.

@boneskull
Copy link
Contributor

@jdalton what's Symbol.for('esm\u200D:package')?

@boneskull
Copy link
Contributor

might be premature to hook in to Node's --loader, but makes sense whenever that becomes stable.

oh, this PR needs to be retargeted to master. I think that means it needs to be closed and reopened. I'm not sure where next came from, but it's probably my fault.

@boneskull
Copy link
Contributor

I think this probably needs to land after v6... I don't think I have time to give it the attention it deserves rn

@boneskull boneskull closed this Jan 29, 2019
@boneskull
Copy link
Contributor

apparently deleting the target branch closes the issue.

@jdalton
Copy link
Author

jdalton commented Jan 29, 2019

what's Symbol.for('esm\u200D:package')?

The esm loader exposes a symbol property flag so AVA, and others, can sniff it to detect the esm loader so they can replace their require() use with the loader instead of treating it like another preload script and just loading it with builtin require().

apparently deleting the target branch closes the issue.

Will re-open under master.

Update:

Ok @boneskull, retargeted. Is it possible to just re-open this PR?

@jdalton jdalton changed the base branch from next to master January 29, 2019 23:00
@boneskull
Copy link
Contributor

doesn’t look like it. ☹️

@jdalton
Copy link
Author

jdalton commented Jan 30, 2019

No worries. Re-opened as #3703.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants