| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, missing docs
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| const { code, signal, stderr, stdout } = await spawnPromisified( | ||
| execPath, | ||
| [ | ||
| '--import', mjsImport, |
There was a problem hiding this comment.
We would want a test with several --import and some way to validate that there's a "main" one
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
We could do both. An --entry-url flag would presumably just take the entry argument and fileURLToPath it (relative to a base of fileURLToPath(process.cwd()). Whereas --import applies the ESM resolution algorithm, so you could do stuff like --import typescript-repl to load a dependency as your main entry point. 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 node by itself or -i / --interactive being passed. So it wouldn’t be just --import, it would be any flag passed to node would cause the need for -i or --interactive to open the REPL. |
Sorry, something went wrong.
|
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 flag too. I'm not seeing how this major break to behaviour and consistency is worthwhile over just having a different, more explicit flag to change how the input is interpreted. |
Sorry, something went wrong.
|
I think node <flags> <no positionals> should definitely continue to open a repl. 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 --import, imo. |
Sorry, something went wrong.
There are already cases where node <flags> <no positionals> doesn’t open the REPL. node --eval '', for example. |
Sorry, something went wrong.
|
@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 node --import=whatever -e ''? |
Sorry, something went wrong.
That’s what #49918 is about. We don’t currently support import.meta.main but that’s a part of this; as I wrote elsewhere, when we add support for import.meta.main it would support this PR by defining the last --import as the main. As discussed in #49918 we might create a new process.entryPoint or the like to define the main entry point separate from exclusively looking at positional arguments. We can also choose to define process.argv[1] with the resolved path of the last --import, for backward compatibility.
Yes, I mentioned that in the top post. But in that case the module that gets import.meta.main, when we create that, would be the eval string. Since older versions of Node don’t support import.meta.main, the --eval '' approach is a good enough equivalent. |
Sorry, something went wrong.
|
I don't quite understand why it's necessary to overload --import for both running a module and pre-loading a module. We do not overload --require for example. The same thing can be achieved with just adding another flag right? What's the necessity of having to introduce a breaking change beyond "we can't think of a better name" or "we just want this name"? |
Sorry, something went wrong.
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 --import makes really clear that “this will follow the same rules as an import statement” that users are already familiar with. The difference between --import and --entry-url, assuming we define the latter as per what’s discussed in #49432, is that --entry-url is a straightforward “convert a file URL with a base of CWD into a file path” whereas --import applies the ESM resolution algorithm, including support for bare specifiers. Maybe we don’t want the ESM resolution algorithm anyway, in which case a new flag is preferable. 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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Yes and yes. That’s why this is a semver-major change. |
Sorry, something went wrong.
|
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. 🤷🏻♂️ |
Sorry, something went wrong.
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 --import? This is subjective, one can also find it surprising that Node.js assumes no additional code should be run after --import, which is like assuming that no additional code need to be run and it's time to exit after you import() in the REPL. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This PR changes the behavior of node --import ./entry.js from running entry.js and then launching the REPL; to running entry.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 like node --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 run node --import ./entry.js?foo=bar or node --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