-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib,test: improves ERR_REQUIRE_ESM message #30694
Conversation
If this get landed, please re-write that commit message before landing, mine is horrible, haha ): |
FIxing tests issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, thanks again for working on this.
This changes are breaking Output: assert.js:561
throw err;
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected
Comparison {
+ code: 'ERR_INVALID_ARG_TYPE',
+ message: 'The "path" argument must be of type string. Received type object'
- code: 'ERR_REQUIRE_ESM',
- message: /Must use import to load ES Module/
}
at Object.<anonymous> (/Users/juanjose/Documents/GitHub/Node/node/test/parallel/test-require-mjs.js:5:8)
at Module._compile (internal/modules/cjs/loader.js:1185:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1205:10)
at Module.load (internal/modules/cjs/loader.js:1040:32)
at Function.Module._load (internal/modules/cjs/loader.js:944:14)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:67:12)
at internal/main/run_main_module.js:17:47 {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: [NodeError],
expected: [Object],
operator: 'throws'
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we're there? Thanks for your persistence on this one!
PR-URL: #30694 Fixes: #30599 Reviewed-By: Guy Bedford <[email protected]>
Landed in 81ac302. |
PR-URL: #30694 Fixes: #30599 Reviewed-By: Guy Bedford <[email protected]>
PR-URL: #30694 Fixes: #30599 Reviewed-By: Guy Bedford <[email protected]>
PR-URL: #30694 Fixes: #30599 Reviewed-By: Guy Bedford <[email protected]>
It introduces a breaking change. |
@pinguinjkeke experimental features in Node.js are not covered by the semver contract. More details about this can be found in our docs We do try and be cautious though. That is part of the reason that we landed this change in a SemverMinor on 12.x and have kept the ESM implementation behind a flag. I apologise for the inconvenience but I hope you understand that being pragmatic about this will allow us to have a more consistent experience across versions of node when we stabilize the api |
Fixes: #30599
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes