Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

007: --entry-extension CLI flag proposal #62

Closed
wants to merge 4 commits into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Aug 9, 2017

No description provided.

@bmeck
Copy link
Member Author

bmeck commented Aug 9, 2017

@rauschma: bikeshed for --input-extension instead

Copy link
Member

@ljharb ljharb left a 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.

@bmeck
Copy link
Member Author

bmeck commented Aug 9, 2017

Mixing anything that is not an opaque text stream such as a filename should error since it doesn't make sense.

@matthewp
Copy link

matthewp commented Aug 9, 2017

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?

@matthewp
Copy link

matthewp commented Aug 9, 2017

Ah, I missed @bmeck's reasoning, nevermind. That's fine I think.

@rauschma
Copy link

rauschma commented Aug 9, 2017

@bmeck: I mostly agree, but executing a .js file as an ES module may be a use case.

@bmeck
Copy link
Member Author

bmeck commented Aug 9, 2017

@rauschma pipe it in over STDIN

@rauschma
Copy link

rauschma commented Aug 9, 2017

@bmeck As that is the only use case I can think of, that’s probably the right call, yes.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2017

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
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

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

Copy link

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 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Member

@ljharb ljharb left a 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?

@bmeck
Copy link
Member Author

bmeck commented Aug 9, 2017

What happens when a leading . is supplied?

Stream goes through CJS require.extensions then ESM import mechanics. It wouldn't find a ..js etc. extension and would fail.

What happens when an unknown extension is supplied?

Fails to load.

Is there a way to list supported extensions on the CLI?

Not currently, but could be proposed.

@bmeck
Copy link
Member Author

bmeck commented Aug 9, 2017

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.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2017

It seems like node --entry-extension=foo should fail immediately if foo isn't in require.extensions, and ideally would print out Object.keys(require.extensions) in the error message.

@bmeck
Copy link
Member Author

bmeck commented Aug 9, 2017

@ljharb I am hesitant to do that error behavior since people could certainly add to the list using --require. Also, require.extensions is not used in the ESM logic.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2017

@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.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

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.

Copy link
Member Author

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

Copy link

@Fishrock123 Fishrock123 left a 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.

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

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.

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.

Copy link
Member Author

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?

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about -x ?

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?

Copy link
Member Author

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`.

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?

Copy link
Member Author

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.

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`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems far less intuitive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, not suggested.

@MylesBorins
Copy link

@bmeck do you think it makes sense to close this as we are no longer using the eps process?

@bmeck
Copy link
Member Author

bmeck commented Nov 29, 2017

@MylesBorins yes, as I presumed all EPs would be considered archived I didn't touch it.

@bmeck bmeck closed this Nov 29, 2017
@styfle
Copy link
Member

styfle commented Nov 29, 2017

007 is dead

007

See what I did there? 😄

......I'll show myself out now 🏃 🚪

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants