-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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: support loading entrypoint as url #54933
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54933 +/- ##
==========================================
- Coverage 88.06% 88.06% -0.01%
==========================================
Files 651 651
Lines 183451 183543 +92
Branches 35821 35863 +42
==========================================
+ Hits 161564 161642 +78
- Misses 15133 15138 +5
- Partials 6754 6763 +9
|
4b36a9e
to
9d90d6c
Compare
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.
The code LGTM.
Adding more tests (and maybe examples in the docs) would be appreciated, and i think it should be more clear that --entry-url
is a binary flag that doesn't require --entry-url <URL>
syntax, i.e. node --any-flags file:///file.mjs --any-flags --entry-url --any-flags
also works.
Also, IIRC src
subsystem exists mainly for generic changes (just like lib
), so just module
should be sufficient.
9d90d6c
to
bebf291
Compare
Co-Authored-By: Antoine du Hamel <[email protected]>
bebf291
to
8233681
Compare
if (mainURL === undefined) { | ||
mainURL = pathToFileURL(mainPath).href; | ||
} | ||
const mainURL = getOptionValue('--entry-url') ? new URL(mainPath, getCWDURL()).href : pathToFileURL(mainPath).href; |
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.
We can avoid repeating the .href
here, and we should probably rename mainPath
.
const mainURL = getOptionValue('--entry-url') ? new URL(mainPath, getCWDURL()).href : pathToFileURL(mainPath).href; | |
const mainURL = getOptionValue('--entry-url') ? new URL(mainPathOrURL, getCWDURL()) : pathToFileURL(mainPathOrURL); |
} | ||
|
||
assert.strictEqual(code, expected.code ?? 0); | ||
assert.strictEqual(signal, expected.signal ?? signal); |
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.
assert.strictEqual(signal, expected.signal ?? signal); | |
assert.strictEqual(signal, expected.signal ?? null); |
{}, | ||
{ | ||
...experimentalFeatureWarning, | ||
stdout: /data:text\/javascript,console\.log\(import\.meta\.url\)/, |
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.
stdout: /data:text\/javascript,console\.log\(import\.meta\.url\)/, | |
stdout: /^data:text\/javascript,console\.log\(import\.meta\.url\)\r?\n$/, |
{ cwd: fixtures.fileURL('./') }, | ||
{ | ||
...experimentalFeatureWarning, | ||
stdout: /print-entrypoint\.mjs\?key=value#hash/, |
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.
stdout: /print-entrypoint\.mjs\?key=value#hash/, | |
stdout: /print-entrypoint\.mjs\?key=value#hash\r?\n$/, |
{}, | ||
{ | ||
...experimentalFeatureWarning, | ||
stdout: /A/, |
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.
stdout: /A/, | |
stdout: /^A\r?\n$/, |
Ref: #49975
Fixes?: #46009
Fixes?: #49204
The original PR stalled, so I addressed the comments, and reimplemented it in a way that it works with the latest Node.js codebase.