Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
esm: allow --import to define main entry#49946
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Sep 29, 2023
Review requested:
|
63104a7 to 3af3746Compare3af3746 to 6ef8d53Compare
MoLow left a comment
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
targos commented Sep 29, 2023
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) |
Qard left a comment
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 }=awaitspawnPromisified( | ||
| 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
benjamingr left a comment
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?
GeoffreyBooth commented Sep 29, 2023
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 |
Qard commented Sep 29, 2023
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 |
isaacs commented Oct 1, 2023
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 |
GeoffreyBooth commented Oct 1, 2023
There are already cases where |
isaacs commented Oct 2, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@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 |
GeoffreyBooth commented Oct 2, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
joyeecheung commented Oct 2, 2023
I don't quite understand why it's necessary to overload |
GeoffreyBooth commented Oct 2, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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. |
Qard commented Oct 2, 2023
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. |
GeoffreyBooth commented Oct 2, 2023
Yes and yes. That’s why this is a semver-major change. |
Qard commented Oct 3, 2023
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. 🤷🏻♂️ |
joyeecheung commented Oct 3, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
aduh95 commented Sep 28, 2024
Superseded by #54933 |
This PR changes the behavior of
node --import ./entry.jsfrom runningentry.jsand then launching the REPL; to runningentry.jsas the main entry point. This is a semver-major change; to preserve the prior behavior, the user must additionally pass-ior--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
--importare parsed as URLs. Users can runnode --import ./entry.js?foo=barornode --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