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.
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: make CJS load from ESM loader #47999
module: make CJS load from ESM loader #47999
Changes from all commits
3deeccf
c1b346c
df14ab8
f5fdea1
598d789
afd882a
9958f1f
6d57350
ad7a8ed
d0e4abf
39219be
739dc9e
297a63b
32bf878
74c93d1
0a63101
a6f01a3
3b39ba3
dd2af79
f692715
850831b
c7edb23
2a5521e
507cd21
ec6d178
279e19d
454985b
2c499bc
2c5fcfb
8291086
624d933
5d9cdd0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
(possibly might need another
?
betweenmanifest
andassertIntegrity
)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.
I copied that from
getSource
, we should probably address both in a follow up PR.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.
Format does not / should not change, it just needs a default value, so I think it would be better to keep it a constant with a default:
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.
What if
format
isnull
?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.
Ah, we have a bug in the
resolve
hook final result validation:node/lib/internal/modules/esm/hooks.js
Lines 336 to 339 in d150316
format
is an enum, sonull
is not valid (only the enum values or nothing)Let's fix them together (we can't fix this part without creating a second bug).
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.
I'm pretty sure we've always accepted nullish values indifferently in the hook APIs.
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.
Even for enums? Why?
null
is an actual value.