-
Notifications
You must be signed in to change notification settings - Fork 66
007: --entry-extension CLI flag proposal #62
Conversation
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 happens with node --entry-extension=js app.js
or node --entry-extension=mjs app.js
?
I would expect either an exception when there's a mismatch, or, that the entry-extension would silently override the file extension.
Mixing anything that is not an opaque text stream such as a filename should error since it doesn't make sense. |
In @ljharb's case I would want/expect the entry-extension to silently override the file extension. If it throws, then node --entry-extension=mjs app.js and node --entry-extension=mjs < app.js Would do different things, and why should they? |
Ah, I missed @bmeck's reasoning, nevermind. That's fine I think. |
@bmeck: I mostly agree, but executing a |
@rauschma pipe it in over STDIN |
@bmeck As that is the only use case I can think of, that’s probably the right call, yes. |
As long as it's a clear decision one way or the other, I'm happy :-) it just can't be unspecified. |
|
||
```sh | ||
# exits with an error | ||
node --entry-extension=json app.js |
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.
node --entry-extension=js app.js
should then also error?
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.
yes
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.
Might be a good example to add so that people don't think it's the mismatch that errors
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.
Definitely a good example since most of the feedback so far has been above this :)
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.
added
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 happens when a leading .
is supplied?
What happens when an unknown extension is supplied?
Is there a way to list supported extensions on the CLI?
Stream goes through CJS
Fails to load.
Not currently, but could be proposed. |
This change does not attempt to change workflow of how files are loaded. It just declares the mode rather than going through CJS always. See https://github.com/nodejs/node/blob/5fd2f03b16bfd6730fe82dc3217f1d05131b9615/lib/internal/bootstrap_node.js#L435 for some existing implementation details. |
It seems like |
@ljharb I am hesitant to do that error behavior since people could certainly add to the list using |
@bmeck in that case i'd expect the list to be runtime-computed, so if they'd added to the list, it would become a valid option. However it's implemented, I think it'll be important to fail-fast when an invalid extension is supplied, and make sure the error message is meaningful and actionable. |
|
||
### Early Errors | ||
|
||
Any unknown file extension should prevent the main entry point to the program from executing anything. Preloaded files such as using `--require` should continue to load normally. |
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.
Is there any concept of preloading files, but with a different entry extension? In other words, --import
to match --require
?
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.
@ljharb not currently, use a wrapper for now. --require
uses file paths always, so file extensions are always included. There are some odd problems with naming here, and some timing differences due to ESM being async that I think are fine to put off for a future time.
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, so --require file.mjs
would work as expected?
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.
maybe? require('./foo.mjs')
throws so the naming is a bit odd but since CLI arg for main file to run accepts .mjs
it should work fine I would expect.
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.
Correct, but require()
has synchronous code boundaries to deal with. We have more freedom in how we define preload's interactions.
Perhaps we should have --import
alias to --require
though, for posterity and to avoid confusion. That being said, -i
is already used.
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.
IDK, --import
seems fine and can use the module resolution of ESM rather than CJS then ESM
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 good overall. Have some comments.
I think this could be a place to define how -r
works with non-Script goal programs, but I wouldn't be opposed to having that discussion in a different place either.
|
||
## Description | ||
|
||
The Node CLI currently accepts input in both filename (`node app.js`) and opaque text stream form (`node -e '123'` and `node <app.js`). This behavior always results in running the stream as a CommonJS goal. With the upcoming usage of ESM and WASM there will be cases where developers wish to pipe other forms of text to the CLI. |
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.
Script goal
and maybe also other program forms to the CLI
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.
On second thought, I suppose that last one could also be clarified as source text
if that is more consistent.
|
||
The Node CLI currently accepts input in both filename (`node app.js`) and opaque text stream form (`node -e '123'` and `node <app.js`). This behavior always results in running the stream as a CommonJS goal. With the upcoming usage of ESM and WASM there will be cases where developers wish to pipe other forms of text to the CLI. | ||
|
||
This proposal is to implement an `--entry-extension` CLI flag for the cases which may want to use a stream that does not have a file extension. |
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 we are defining the name here, might be worthwhile to also specify --ext
as a shorter-hand. People will want to see using it as reasonable UX.
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.
do we have any abbreviates --
flags?
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.
Unsure but I don't really see an issue with it.
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 about -x
?
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.
Could use that, I suppose. Does it have any common UNIX meaning it might be confused with?
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.
|
||
### Userland Extensions | ||
|
||
Some libraries extend `require.extensions` with new extensions. These libraries should be considered when determining the list of extensions allowed in `--entry-extension`. |
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.
How would this work? Only for preload modules I suppose?
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.
Yep. Only preload would affect this.
|
||
### Early Errors | ||
|
||
Any unknown file extension should prevent the main entry point to the program from executing anything. Preloaded files such as using `--require` should continue to load normally. |
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.
Correct, but require()
has synchronous code boundaries to deal with. We have more freedom in how we define preload's interactions.
Perhaps we should have --import
alias to --require
though, for posterity and to avoid confusion. That being said, -i
is already used.
|
||
## Alternate Possibilities | ||
|
||
Some alternate possibilities exist that might be relevant. `--entry-url` for example could be used to provide an `import.meta.url` properly while also providing the pathname that contains an extension. However, since URLs are not mandated to have file extensions this might be for nothing. Applications can also access `process.cwd()` to recreate similar data to `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.
Seems far less intuitive?
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.
yup, not suggested.
@bmeck do you think it makes sense to close this as we are no longer using the eps process? |
@MylesBorins yes, as I presumed all EPs would be considered archived I didn't touch it. |
No description provided.