-
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
esm: improve fetch_module
test coverage and remove hack
#41947
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,48 @@ | ||||||||||||||||||||||||||||||||||||||
import { mustCall } from '../common/index.mjs'; | ||||||||||||||||||||||||||||||||||||||
import { match, notStrictEqual } from 'assert'; | ||||||||||||||||||||||||||||||||||||||
import { spawn } from 'child_process'; | ||||||||||||||||||||||||||||||||||||||
import { execPath } from 'process'; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||
const child = spawn(execPath, [ | ||||||||||||||||||||||||||||||||||||||
'--experimental-network-imports', | ||||||||||||||||||||||||||||||||||||||
'--input-type=module', | ||||||||||||||||||||||||||||||||||||||
'-e', | ||||||||||||||||||||||||||||||||||||||
'import "http://example.com"', | ||||||||||||||||||||||||||||||||||||||
]); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let stderr = ''; | ||||||||||||||||||||||||||||||||||||||
child.stderr.setEncoding('utf8'); | ||||||||||||||||||||||||||||||||||||||
child.stderr.on('data', (data) => { | ||||||||||||||||||||||||||||||||||||||
stderr += data; | ||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||
child.on('close', mustCall((code, _signal) => { | ||||||||||||||||||||||||||||||||||||||
notStrictEqual(code, 0); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// [ERR_NETWORK_IMPORT_DISALLOWED]: import of 'http://example.com/' by | ||||||||||||||||||||||||||||||||||||||
// …/[eval1] is not supported: http can only be used to load local | ||||||||||||||||||||||||||||||||||||||
// resources (use https instead). | ||||||||||||||||||||||||||||||||||||||
match(stderr, /[ERR_NETWORK_IMPORT_DISALLOWED]/); | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I prefer the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have access to node/test/es-module/test-esm-import-json-named-export.mjs Lines 7 to 24 in 15f1a45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gah, yes indeed (and aligns with #41352) |
||||||||||||||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||
const child = spawn(execPath, [ | ||||||||||||||||||||||||||||||||||||||
'--experimental-network-imports', | ||||||||||||||||||||||||||||||||||||||
'--input-type=module', | ||||||||||||||||||||||||||||||||||||||
]); | ||||||||||||||||||||||||||||||||||||||
child.stdin.end('import "http://example.com"'); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let stderr = ''; | ||||||||||||||||||||||||||||||||||||||
child.stderr.setEncoding('utf8'); | ||||||||||||||||||||||||||||||||||||||
child.stderr.on('data', (data) => { | ||||||||||||||||||||||||||||||||||||||
stderr += data; | ||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||
child.on('close', mustCall((code, _signal) => { | ||||||||||||||||||||||||||||||||||||||
notStrictEqual(code, 0); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// [ERR_NETWORK_IMPORT_DISALLOWED]: import of 'http://example.com/' by | ||||||||||||||||||||||||||||||||||||||
// …/[stdin] is not supported: http can only be used to load local | ||||||||||||||||||||||||||||||||||||||
// resources (use https instead). | ||||||||||||||||||||||||||||||||||||||
match(stderr, /[ERR_NETWORK_IMPORT_DISALLOWED]/); | ||||||||||||||||||||||||||||||||||||||
})); | ||||||||||||||||||||||||||||||||||||||
} |
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 reason I added this was because the output without it seemed likely to confuse (the file path is irrelevant and makes it took like it's coming from a file, but it's not). Aside from consistency with CJS, I think this is not an improvement (I don't feel strongly though).
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.
If the URL is confusing, we should fix the URL itself, not try to hide it in this specific error message imo. I disagree that
command-line
is less confusing – what if the user didn't start the process via the command-line?Note that the condition can never be true:
parentName
is never equals to[eval]
nor[stdin]
(it could be[eval1]
though, but we're not testing that).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.
Sure! That works for me (and should also better support consistency).
Is that possible?
If it doesn't now, it did at the time—I specifically tested CLI via
-e …
, which is how I found this in the first place 🙂 Is there a known related change that would explain why it's not now?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.
Sure, why not? I don't have a deep enough knowledge of the various OSs Node.js supports to list them all, but I know that CLI is just an option among others (from the GUI, as a service, from a parent process, etc.).
Maybe you forgot to add
--input-type=module
?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.
No, I mean, is there a route that exists that leads to
[eval*]
/[stdin]
that did not originate from CLI?I did not 🙂