fix: ensure require.resolve is only ever called with absolute filepaths

[ctjlewis]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions.

Description of the Change

unloadFile is only ever passed filepaths, but adding a file with mocha.addFile(relativePath) throws MODULE_NOT_FOUND in an ESM context because require.resolve(relativeFile) fails.

Ensuring that unloadFile only ever calls require.resolve with absolute filepaths by wrapping with path.resolve(relativePath) prevents this error.

Applicable issues

• Closes #4548

[juergba]

@ctjlewis thank you I did some changes to our CI test workflow yesterday. I don't know why the pull-request event hasn't triggered our CI tests on your PR. I need to get this solved first.

[ctjlewis]

No worries @juergba, I was looking for CI tests as well. I will pull from upstream when that's settled, just ping me here, no rush.

[coveralls]

coverage: 94.338% (+0.003%) from 94.335% when pulling 3b4e15aee8e211b25777a66bb3abd96646f3e772 on ctjlewis:unload-files into 53a4bafbdeb32576440b0a21787f2525585411c0 on mochajs:master.

[juergba]

So, I have two problems with the workflow: I don't know your branch's name from your fork. It's not unload-files, nor ctjlewis:unload-files, nor ctjlewis/mocha:unload-files. I need to distinguish PR's coming from a fork against pushed ones to the original mocha repo.

Browser test: The github docs says:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

I guess thats the reason, why the browser test is failing. The SaucLabs login doesn't work. Do you have any idea?

[ctjlewis]

GitHub embeds it here as ctjlewis/mocha:unload-files and ctjlewis:unload-files.

I have no idea about the CI - I've never contributed to Mocha before, I just ran into this error and wanted to submit a patch.

[juergba]

The CI tests do run now on your PR, but the browser test will fail most propably. I will have to investigate further on that one.

You could rebase locally and force-push to your fork, if you want. Thanks for letting me abuse your PR.

[ctjlewis]

No worries @juergba, feel free to use this PR to debug.

[ctjlewis]

@juergba Hey, any way I can help out here? This bug does actually break most unloadFiles() calls in ES contexts - not trying to be pushy at all, but I'm happy to help move it along any way I can. This was the fastest response to a PR I've ever had (1hr) so I'm not complaining by any means.

[juergba]

@ctjlewis If I remember correctly, the ESM loader doesn't use the require cache.

see our API - unloadFiles see Node docs

Even worse, AFAIK there is no way to clear the ESM cache. E.g. that's why our watch functionality doesn't work with ESM. There is no way to clear the cache and re-run the tests. BTW unloadFile is a private function.

EDIT: Adding a file does not mean it is loaded. With ESM Modules you have to load the files asyncly with loadFilesAsync.

[ctjlewis]

This PR fixes the issue. And yes but unloadFiles, which relies unloadFile, is public.

I ran into this issue in real-world codebase, and resolving the import path works. Mocha is imported as a CJS module so require works as expected inside Mocha’s source, it can be safely ignored.

On Wed, Jan 13, 2021 at 8:22 AM Juerg B. [email protected] wrote:

@ctjlewis https://github.com/ctjlewis If I remember correctly, the ESM loader doesn't use the require cache.

see our API - unloadFiles https://mochajs.org/api/mocha#unloadFiles see Node docs https://nodejs.org/docs/latest-v14.x/api/esm.html#esm_no_require_cache

Even worse, AFAIK there is no way to clear the ESM cache. E.g. that's why our watch functionality doesn't work with ESM. There is no way to clear the cache and re-run the tests. BTW unloadFile is a private function.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mochajs/mocha/pull/4549#issuecomment-759480080, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMUTFA3GRTV6U63N5W4NL3SZWUA5ANCNFSM4V5GQ62A .

[juergba]

I'm not convinced of this change. I will need a few days to look into it more closely.

Your examples just don't make any sense:

• you never load any test files, neither the ESM way via loadFilesAsync, nor the CommonJS way via mocha.run (which calls loadFiles). The function loadFiles does call path.resolve.
• unloadFiles does not clear the ESM cache (which is not the require.cache).

[ctjlewis]

I guess the issue is not as obvious as I thought - I made a Repl.it here: https://repl.it/@christiantjl/MochaRequireResolve

This basically only affects programs where we import Mocha from an ESM context. ES modules can import CJS modules, and that require.resolve statement is executed within the emulated CJS module. I think under the hood it relies on import.meta.resolve but since Mocha is a CJS module, this is the appropriate fix, which adds support for ES interoperation.

I should have made it clear this is an ESM interop thing, where, for CJS programs, this will have no impact, but it will prevent this error from presenting when Mocha is used programmatically from inside an ES module via import Mocha from 'mocha'.

Apologies for not including a repro off the bat.

[ctjlewis]

Tfw I did include a repro in that issue, and just recreated it a second time four days later... There are now two repros:

1. https://repl.it/@christiantjl/MochaRequireResolve
2. https://repl.it/@christiantjl/MochaFileUnloader

I would start with 1. which is the most recent.

[ctjlewis]

Hate to bug you @juergba, but did you have a chance to review the example?

[juergba]

@ctjlewis None of your repros works, there are syntax errors all over when pressing the Run-Button.

Meanwhile I doubt that this issue has its cause specifically with ESM. If you just take following code snippet:

mocha.addFile('./test/test-pending.js');
console.log(require.resolve('./test/test-pending.js', {paths:[process.cwd()]}));     // runs successfully
console.log(require.resolve('./test/test-pending.js'));                              // fails

The module resolution algorithm does not include sub-directories of the current working directory. Therefore require.resolve fails.

Since loadFiles calls path.resolve, it could make sense to do the same in unloadFiles as well.

[ctjlewis]

The run button doesn't work because of the Node version Repl.it uses by default (does not support ESM yet - see How to import an ES module in Node.js REPL?). It does in the shell - execute node index.js to show the behavior of succeeds.js and fails.js. I assure you I tested both repros before sending and they are definitely both perfectly functional ECMAScript programs.

There are not "all kinds of syntax errors" when you press Run either, there is one: Cannot use import statement outside a module, which is self-explanatory. It indicates Node is not treating the module as an ES module because of Repl.it limitations. I am shocked you could not take it from there and just execute the modules directly from the shell, rather than come back and say the repros "don't work." It is a visibly valid ES module.

I am admittedly getting frustrated because you keep trying to tell me that this error is not real simply because you are struggling to understand it, and it is bordering on intentionally wasting my time. I have tried to explain this multiple times: This error presents when Mocha is imported as a CJS module in an ESM context.

In the scope where Mocha is imported via:

import Mocha from 'mocha'

We cannot actually interface with require. Only inside the Mocha module, which is CJS, do the require calls work. This works almost perfectly in an ES context, except for this issue which I am trying to close with this PR. I am actually running a patched local version of Mocha right now, so that my ESM test containing an unloadFiles() call does not throw.

Besides the changes in this PR, in theory, I could rename the file to test.cjs and rewrite it as a CJS module (const Mocha = require('mocha') instead of import Mocha from 'mocha') and the issue would go away, because it would no longer be executing Mocha calls in an ES context. Does that make sense?

And to clarify the nature of the error, it presents when we do:

// ES context
import Mocha from 'mocha'

// relative path
mocha.addFile('relative/path/to/file.js');

// throws MODULE_NOT_FOUND, relative path in ES context
mocha.unloadFiles();

But not when we resolve the path just to make the guarantee that the resolve.require() call will never be passed a relative directory:

// ES context
import Mocha from 'mocha'

// absolute path
mocha.addFile(path.resolve('relative/path/to/file.js'));

// succeeds
mocha.unloadFiles();

[ctjlewis]

CC: @boneskull @craigtaub @Munter @outsideris

[ctjlewis]

I updated the Repl.it with a custom run command and .mjs extensions to get the repro to work with the Run button rather than needing to execute it directly:

https://repl.it/@christiantjl/MochaRequireResolve

[github-actions[bot]]

This PR hasn't had any recent activity, and I'm labeling it stale. Remove the label or comment or this PR will be closed in 14 days. Thanks for contributing to Mocha!

[JoshuaKGoldberg]

Looks like this was not stale, and the author did provide a good amount of repro steps. Re-opening & marking the issue as accepted.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: ctjlewis / name: Lewis (3b4e15aee8e211b25777a66bb3abd96646f3e772)

[JoshuaKGoldberg]

👋 ping @ctjlewis, is this still something you have time for?

[JoshuaKGoldberg]

Closing out as it's been a while since PR activity. If anybody wants to take this over, please do - and post a co-author attribution if you take code from this PR. Cheers! 🤎