-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: allow --import
to define main entry
#49946
Conversation
Review requested:
|
63104a7
to
3af3746
Compare
3af3746
to
6ef8d53
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.
LGTM, missing docs
I don't really like the idea of a flag that has a different meaning based on the presence of other arguments and that behaves differently if it's the last occurrence. I don't see a problem with adding a new flag (and to me it's better if we can backport the feature and avoid a breaking change) |
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.
Not a fan of the inconsistent behaviour this causes with the repl. I would much prefer just adding a new flag to tell it to interpret as a URL. Either the previously suggested --entry-url flag or something like --entry-mode=url.
const { code, signal, stderr, stdout } = await spawnPromisified( | ||
execPath, | ||
[ | ||
'--import', mjsImport, |
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 would want a test with several --import
and some way to validate that there's a "main" one
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.
Weren't we telling people to use --import
in order to run logic before any code runs? Wouldn't overloading it to do both that and to allow an esm/url entry point from the CLI be kind of confusing?
We could do both. An Maybe there’s not much value in the latter, I don’t know. I was also trying to not continue to expand our ever-growing list of flags. If you don’t like the inconsistency we could just limit the REPL opening to either |
That'd be quite a break from the existing behaviour for limited benefit, and there are plenty of valid use cases to pass other flags to the repl. No doubt there are existing scripts out there to start the repl which assume omitting an entrypoint is enough and therefore omit the |
I think It'd be a pretty significant break from the past to change that. Adding more flags isn't great, I agree, but it seems less confusing to expand the list than overload |
There are already cases where |
@GeoffreyBooth that is true, sorry, I was imprecise. What I meant was, they aren't the normal "run a main" style of node invocation. Having a main module, and also process.argv[1] is undefined, feels super weird. Can't you already kind of do this with |
That’s what #49918 is about. We don’t currently support
Yes, I mentioned that in the top post. But in that case the module that gets |
I don't quite understand why it's necessary to overload |
Sure, but at some point all those flags add up. And it’s not a matter of not thinking of a better name, is that’s a name of The difference between I don’t have a preference between the two options; I opened this first because it would be semver-major and we’re approaching that deadline. We can also ship both, if we want to provide a way to define the entry point via the ESM resolution algorithm and via a simple file URL; I would be fine with that too. It doesn’t really matter, though, if @Qard (or anyone else who isn’t blocking because he’s already blocked) is opposed to any of these options. |
That's a redefinition of what the import flag means though. I think it's generally not a good idea to redefine already existing flags. Like I said, there's almost certainly already code out there using the flag and expecting it to bootstrap some code into a repl, not to become the entrypoint itself. |
Yes and yes. That’s why this is a semver-major change. |
That's also why I don't think it's a change worth making. It would be confusing to users with not really any benefit that can't be better solved by a different flag. 🤷🏻♂️ |
I don't think this is a strong enough argument for a breaking change. If a flag is taken, it's taken. We can't change the meaning of the flags just because we think we come up with a better idea for the moment, because other ideas can still come up in the future and we can't just keep mutating the meaning of the same flag whenever we think we have a better idea.
How is that in conflict with opening up REPL if there's nothing else following |
Superseded by #54933 |
This PR changes the behavior of
node --import ./entry.js
from runningentry.js
and then launching the REPL; to runningentry.js
as the main entry point. This is a semver-major change; to preserve the prior behavior, the user must additionally pass-i
or--interactive
, so likenode --import ./entry.js --interactive
.There are a few reasons for this change in behavior:
It provides a way to run a URL as the main entry point, because the values of
--import
are parsed as URLs. Users can runnode --import ./entry.js?foo=bar
ornode --import data:text/javascript,console.log("Hello")
.It provides a way to run a bare specifier as the main entry point, for example
node --import typescript-repl
.This satisfies the “define the main entry point as a URL” part of #49432.
Since this is a semver-major change, it cannot be backported.
node --import ./entry.js?foo=bar --eval ''
can achieve very similar results in current and past versions of Node. See also #49432 (comment). @nodejs/loaders