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

Fixes #4803: wrong error thrown if loader is used #4807

Merged
merged 2 commits into from Jan 10, 2022

Conversation

giltayar
Copy link
Contributor

@giltayar giltayar commented Jan 4, 2022

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

When loading test files, there is a heuristic that tries to determine whether to import or require them. This heuristic failed in the presence of a loader that recognizes filenames that are not .js/.mjs. In this case, when require-ing the file, Node.js does not throw ERR_REQUIRE_ESM (because of the different extension) but rather a SyntaxError saying that a CJSJ cannot use import.

I added some more code to this heuristic to deal with the problem by checking if the error is the aforementioned syntax error, and if so, throw the original error.

Alternate Designs

Couldn't think of any.

Why should this be in core?

Well... this is the code of the loading mechanism in Mocha.

Benefits

Devs that use ESM with loaders will get the correct error and not a weird error that they can't use to understand what the problem is.

Possible Drawbacks

Heuristics, as they go, should be tampered with lightly, given that they may succeed in all our tests, but fail in the wild world out there.

I have been careful to add tests to all weird cases I've seen, so I'm pretty confident of the change, but as I said, this may be problematic.

Applicable issues

Not a breaking change.

@coveralls
Copy link

@coveralls coveralls commented Jan 4, 2022

Coverage Status

Coverage remained the same at 94.428% when pulling b37be07 on giltayar:esm-loader-wrong-err into 60fafa4 on mochajs:master.

@giltayar
Copy link
Contributor Author

@giltayar giltayar commented Jan 4, 2022

Fixes #4803

@juergba juergba added node.js semver-patch labels Jan 4, 2022
@juergba juergba added this to the next milestone Jan 4, 2022
@JakobJingleheimer
Copy link

@JakobJingleheimer JakobJingleheimer commented Jan 4, 2022

Thank you!

@giltayar
Copy link
Contributor Author

@giltayar giltayar commented Jan 4, 2022

@JakobJingleheimer You're doing important work in Node.js on the loaders. So thank you. 🙏

@JakobJingleheimer
Copy link

@JakobJingleheimer JakobJingleheimer commented Jan 6, 2022

Sorry, not sure if you were waiting for me to verify the fix. Just in case:

I replaced my local node_modules/mocha/lib/nodejs/esm-utils.js with the one from this PR and re-ran the test, and it correctly errors with

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'i-dont-exist' imported from /…/test.jsx

@juergba juergba linked an issue Jan 10, 2022 that may be closed by this pull request
4 tasks
Copy link
Member

@juergba juergba left a comment

@giltayar thank you

@juergba juergba merged commit baa12fd into mochajs:master Jan 10, 2022
25 checks passed
@giltayar giltayar deleted the esm-loader-wrong-err branch Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node.js semver-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants