ESM swallowing MODULE_NOT_FOUND errors in case of type:module #4687
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👍 |
juergba
added
type: bug
juergba
reviewed
Jul 12, 2021
juergba
added
the
run-browser-test
github-actions
bot
removed
the
run-browser-test
juergba
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
The move to "import-first" in Mocha generates a subtle problem in case of test files that are
.js, yet are ESM due to the package.json include a"type": "module"property: if the test file includes a bad import, then Node.js throws anERR_MODULE_NOT_FOUND, which we catch and try torequirethe module. Because the extension is.jsand we have"type": "module", Node.js tries to be helpful and throws anERR_REQUIRE_ESM, which is what we throw back to the user.As pointed out by @GerHobbelt, this is totally confusing, as the original error with the correct information to resolve the problem is thrown away, and replaced by an error that has no bearing on the original problem.
What we do in the PR is simple: we figure out that this is the situation, and if it is, we throw the original error (
ERR_MODULE_NOT_FOUND) instead of the new one (ERR_REQUIRE_ESM).Alternate Designs
We thought about throwing a new error with a message that includes both error call stacks. But that's ugly and will also confuse the user, because they need to understand which of both errors is really relevant, and will be confused by an error who's message includes call stacks.
Benefits
Less confusion!
Possible Drawbacks
We're still "swallowing" errors in other situations. The alternate design would have taken care of them. We still chose the above design because it is better for the user, and currently appears to be the only problematic case.
Applicable issues
#4675, and than you to @GerHobbelt for the great bug explanation and repo that reproduces the problem. They really helped!
Mocha follows semantic versioning: http://semver.org
Is this a breaking change (major release)? No
Is it an enhancement (minor release)? No
Is it a bug fix, or does it not impact production code (patch release)? Yes