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

import-first loading of test files #4635

Merged
merged 6 commits into from
May 30, 2021
Merged

Conversation

giltayar
Copy link
Contributor

Description of the Change

Test files can be either CJS or ESM. Up to now, we used the following method to support both: first require, and if that fails with ERR_REQUIRE_ESM, then use import.

This was a necessity so long as some supported Node versions did not support ESM. Now that all supported versions of Node support ESM, we can go to a better import-first method, whereby we first try using import(file) and only if that fails, do we try require.

This is a big change, which is theoretically transparent, but could result in unforeseen behavior. We should do this in a SEMVER_MAJOR change.

Alternate Designs

Figure out whether the file is CJS or ESM: unfortunately, the algorithm is complex and basically should replicate Node's, which is practically impossible.

Why should this be in core?

Because no plugin can do this.

Benefits

Allowing ESM loaders to transform a file.

Possible Drawbacks

Backward compatibility: are there any scenarios which we have not foreseen, that fail when we do import first? I couldn't think of any, but there could be. That is why this should be a SEMVER_MAJOR change.

@coveralls
Copy link

coveralls commented May 21, 2021

Coverage Status

Coverage increased (+0.06%) to 94.336% when pulling 35a5c9f on giltayar:remove-node-10-esm into 1b5cbf1 on mochajs:master.

@juergba
Copy link
Contributor

juergba commented May 21, 2021

@giltayar I have some questions:

  • minimum Node version: our CI tests have been running on Node v12.22. Which is your minimum requirement on v12?
  • Node's native ESM implementation was declared stable in v15.3. So below this version we are using an experimental feature for the import of all our test files, rigth? Isn't this risky?
  • our require option is also affected by this change. No risk? requiring a modul without indicating the file exension?
  • really no risks?: different, uncomprehensive error messages? missing file extensions? CJS loaders(?)
  • loading of files *.ts or *.jsx ?

Allowing ESM loaders to transform a file.

Is this remark linked to eg. *.ts or *.jsx files?

We should align our docs as well.

@juergba juergba added area: node.js command-line-or-Node.js-specific type: cleanup a refactor semver-major implementation requires increase of "major" version number; "breaking changes" labels May 21, 2021
@juergba juergba added this to the v9.0.0 milestone May 21, 2021
@juergba juergba requested a review from a team May 21, 2021 13:16
@giltayar
Copy link
Contributor Author

@juergba

minimum Node version: our CI tests have been running on Node v12.22. Which is your minimum requirement on v12?

According to the documentation, v12.17.0 (see image below for history of Node ESM)

Node's native ESM implementation was declared stable in v15.3. So below this version we are using an experimental feature for the import of all our test files, rigth? Isn't this risky?

All ESM functionality was backported to v12, and is stable from v12.22.0:
image

our require option is also affected by this change. No risk? requiring a module without indicating the file exension?

A few tests in the mocha suite actually caught me when I did a naive implementation, which was to just use import(file), because import(...) must have an extension even if it's loading a CJS file (I didn't know that!). To deal with that, the code catches the error, and try to use require(file) as a fallback (see esm-utils.js).

really no risks?: different, uncomprehensive error messages? missing file extensions? CJS loaders(?)
loading of files *.ts or *.jsx ?

uncomprehensive error messages: We had a problem with showing syntax errors on ESM files imported, which is a v8 bug, which I worked around using a hackish solution. The bug in v8 is still there, as is the workaround. Fortunately (and I checked!), the problem does not exist when import(...) imports a CJS file.

missing file extensions: see above.

CJS loaders: if import imports a CJS file, then it's going through the regular require mechanism, so CJS "loaders" (like babel-register) should work.

loading of files *.ts or *.jsx ?

This is done using CJS loaders aka "register hooks", and is not a problem (see above)

Is this remark linked to eg. *.ts or *.jsx files?

Yes. at my company we use an ESM loader I wrote that is the equivalent of babel-register, but for ESM (https://www.npmjs.com/package/babel-register-esm). The problem is that because require is done first, it gets an ERR_SYNTAX_ERROR on the ts file and thus does not fallback on import. This was the trigger for me to think that we should go ESM first.

We should align our docs as well.

Not sure why: this should be completely transparent to the user.

Note: I would completely understand if we wait with this PR a few months, for the next semver-major release, so that more ESM experience is out there, and ESM is used more. I have a hack at my company that rewrites esm-utils after install so that it is ESM-first, which means I can live with the current situation. But what I experience, others will, especially once ESM loaders will become more common. It can wait a few months, but I'm guessing more people will experience the problem I've experienced in a couple of months.

@juergba
Copy link
Contributor

juergba commented May 22, 2021

It's a good idea to switch to import-first, but limiting our package engine to >=v12.22 (stable) seems too restricting to me. Otherwise waiting for Node-v12's end-of-life in 2022-04-30 isn't an option either.
Can we implement >=v12.22 in esm-utils instead of package engine? Or too complex?

Not sure why: this should be completely transparent to the user.

Our docs still declares Node's native ESM support as experimental.
We should mention that importing a file requires a file extension. Yes, there is your fallback, but anyway. Fallbacks are nice, but depending on them is ugly.
Also the option of using ESM loaders doesn't seem obvious to me.

@giltayar
Copy link
Contributor Author

@juergba

Can we implement >=v12.22 in esm-utils instead of package engine? Or too complex?

I think you're right. It's a good idea. Will do that now in this PR.

Our docs still declares Node's native ESM support as experimental.

I'll change the docs in this PR.

We should mention that importing a file requires a file extension. Yes, there is your fallback, but anyway. Fallbacks are nice, but depending on them is ugly.

This will probably break some people (and will mean I need to fix some of the tests in the test suite). I think leaving the fallback is a good idea. But your call: I'm pretty indifferent.

Also the option of using ESM loaders doesn't seem obvious to me.

Not sure I understood what you mean.

Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

I think we don't need utils.supportsEsModules() any longer, and Mocha.prototype.loadFilesAsync could be simplified as well. IMO there is no need to check for browser, as shown by Mocha.prototype.loadFiles.

lib/esm-utils.js Outdated Show resolved Hide resolved
giltayar added 6 commits May 28, 2021 08:57
FIrst try ESM, and only if that fails, load test file using require.
This enables an ESM loader to have first dibs at transforming the file.
Also removed outdated information and references to
the "esm" module that emulated ESM in Node.js
@giltayar giltayar force-pushed the remove-node-10-esm branch from ed1f4a0 to 35a5c9f Compare May 28, 2021 05:57
@giltayar
Copy link
Contributor Author

@juergba I agree that the code needs refactoring to remove cruft. I actually started writing this PR that way, but found out that the removal of the cruft obscures the main point of this PR, which is doing the loading using ESM-first.

I would gladly contribute another PR after this one to refactor the code once ESM and async are the main ways we load the test code.

@juergba
Copy link
Contributor

juergba commented May 28, 2021

I would gladly contribute another PR after this one to refactor the code once ESM and async are the main ways we load the test code.

ok, I agree.

I have to do some testing with Mocha's programmatic usage. What are the implications of this PR?
Has mocha.loadFilesAsync() to be called before mocha.run() now in any case? Has Mocha.prototype.loadFiles() gotten obsolete?
This would really be a big change, we have to be careful about.

@juergba
Copy link
Contributor

juergba commented May 28, 2021

We still need Mocha.prototype.loadFiles() for:

  • watch mode: no changes here, since Mocha's watcher does not support ESM.
  • programmatic usage: default file loader, but user can optionally switch to the public Mocha.prototype.loadFilesAsync().

So I guess this PR looks fine so far.
@giltayar Should we merge, next Sunday?

@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label May 28, 2021
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label May 28, 2021
Copy link
Contributor

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@giltayar I'm going to merge tonight.

@giltayar
Copy link
Contributor Author

@juergba mocha.loadFiles uses require directly, so no problems there.

@giltayar
Copy link
Contributor Author

@juergba yes, it is a big change, and we should be wary, but given that this will be in a semver-major release, hopefully we'll get early signs of a disaster before everybody gets the version, and we'll be able to revert (or quickly fix).

Thank you for the trust. And for a HUGE load of tests that make me feel confident that it will be alright.

@juergba juergba merged commit 9b4435d into mochajs:master May 30, 2021
@giltayar giltayar deleted the remove-node-10-esm branch May 30, 2021 17:25
@juergba
Copy link
Contributor

juergba commented Jun 1, 2021

@giltayar do you have any wishes regarding release notes? I imagine that many users could be confused with import-first loading of test files wether they have to adjust something or not. Some comforting info could be helpful.

I would gladly contribute another PR after this one to refactor the code once ESM and async are the main ways we load the test code.

Do you want this to be part of v9.0.0?

@giltayar
Copy link
Contributor Author

giltayar commented Jun 1, 2021

do you have any wishes regarding release notes?

Let me have a go at this:

Mocha is going ESM-first! This means that it will first use import(test_file) to load the test files, and only in the rare cases where this fails will it fallback to require(...). This ESM-first approach is the next step in our ESM migration, and allows the ESM loader that runs to load and transform the test file.

Do you want this to be part of v9.0.0?

Since this is a refactor only, It could be part of a later release. It all depends when you want to release 9.0.0, as I will be able to do this PR only starting next week.

@juergba
Copy link
Contributor

juergba commented Jun 1, 2021

I'm planning to publish v9.0.0 this weekend and will add your text to the release notes. I'm missing the info that import is able to load both ESM and CJS test files, and therefore dear users: don't worry, be happy.

@giltayar
Copy link
Contributor Author

giltayar commented Jun 1, 2021

@juergba you're right about import also importing CJS. Here's my change:

Mocha is going ESM-first! This means that it will now use ESM import(test_file) to load the test files, instead of the CommonJS require(test_file). This is not a problem, as import can also load all files that require does, except for rare cases. In the rare cases where this fails it will fallback to require(...). This ESM-first approach is the next step in Mocha's ESM migration, and allows ESM loaders to load and transform the test file.

@juergba
Copy link
Contributor

juergba commented Jun 4, 2021

@giltayar When we load a test file test.ts, then import fails due to the .ts extension and the require fallback is used.
So how does your ESM loader (compiling TS on the fly) work? import is not working for *.ts files?
Can TS be both, ESM and CJS as well?

There is an experimental ts-node/esm loader which transpiles ESM/TS files, no problems with that one either?

Edit: ts-node/esm seems to work as before if --loader is set in .mocharc.cjs. Fails as before if set in NODE_OPTIONS due to Mocha's extensionless binaries.

@giltayar
Copy link
Contributor Author

giltayar commented Jun 6, 2021

When we load a test file test.ts, then import fails due to the .ts extension and the require fallback is used.

Not if there's a loader that loads .ts files (like the loader you mentioned above). If there's a loader that deals with .ts files, it will transform it into a regular JS file and succeed. That is essentially why we're switching the order between import and require.

@juergba
Copy link
Contributor

juergba commented Jun 6, 2021

@giltayar I'm not going to publish v9.0.0 today. I have struggled for hours with transpiling issues, regenerator-runtime intimacies and npm install not creating the .bin folder correctly. I'm sick of this for now.

@brettz9
Copy link

brettz9 commented Jun 7, 2021

I have struggled for hours with...regenerator-runtime intimacies

This may be of little use for your needs, @juergba , but FWIW, I have found https://www.npmjs.com/package/babel-plugin-transform-async-to-promises to work well for my needs without adding its dependency.

@cspotcode
Copy link
Contributor

Just a heads-up that node will be making some breaking changes to the unstable ESM loaders API soon which may break users.

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 semver-major implementation requires increase of "major" version number; "breaking changes" type: cleanup a refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants