-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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
module: expose module format by module loader #57777
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
43f6232
to
e1ee16d
Compare
Module loaders may change a requested module format. It is not safe to determine a `.js` format by `package.json#type` as well. Expose a helper function to frameworks that load js files to avoid relying on `import`/`require` failover-and-retry.
e1ee16d
to
07f897a
Compare
I have a WIP for that in #55756 |
Ah, that's cool! Would you mind adopting tests in this PR as well? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57777 +/- ##
==========================================
+ Coverage 90.23% 90.26% +0.03%
==========================================
Files 630 630
Lines 185245 185253 +8
Branches 36301 36307 +6
==========================================
+ Hits 167154 167223 +69
+ Misses 11006 10982 -24
+ Partials 7085 7048 -37
🚀 New features to boost your workflow:
|
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.
LGTM thanks for working on this ❤️
Module loaders may change a requested module format. It is not safe to
determine a
.js
format bypackage.json#type
as well. Expose a helperfunction to frameworks that load js files to avoid relying on
import
/require
failover-and-retry.Taking mocha as an example: https://github.com/mochajs/mocha/blob/b0c269616e775689f4f28eedc0a9c5e99048139b/lib/nodejs/esm-utils.js#L41-L53.
import
has a different resolution method compared torequire
. So mocharelies on
import
and failover torequire
. But this approach fails when--experimental-strip-types
is enabled by default. For a legacy TypeScriptmodule file, its module syntax should not be depended on. And in the case
of mochajs/mocha#5314, given that
strip-types
isenabled, the error becomes
SyntaxError
and the failover bails out.Frameworks like mocha should take advantage of Node.js module format
detection (including custom loaders) to use
import
orrequire
deterministically.