Skip to content
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

process: argv1 property to preserve original argv[1] #49918

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GeoffreyBooth
Copy link
Member

Following up #49869, supporting #49432, this PR creates a new process.argv1 property similar to process.argv0.

Just as process.argv0 preserves the original value of argv[0] before Node replaces it with the absolute path to the executable, process.argv1 preserves the original value of argv[1] before Node replaces it with the absolute path to the entry point.

There are also some refactors to how some of the “pre-execution” functions interact. I refactored options objects into parameters, as I’m told that generally parameters are faster; and the post-replacement argv[1] is returned to the caller, rather than the caller relying on the process.argv[1] global. I also added some JSDoc and comments. @nodejs/startup

@GeoffreyBooth GeoffreyBooth added process Issues and PRs related to the process subsystem. cli Issues and PRs related to the Node.js command line interface. labels Sep 28, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

lib/internal/modules/run_main.js Outdated Show resolved Hide resolved
lib/internal/modules/run_main.js Outdated Show resolved Hide resolved
lib/internal/modules/run_main.js Outdated Show resolved Hide resolved
lib/internal/process/pre_execution.js Outdated Show resolved Hide resolved
doc/api/process.md Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
GeoffreyBooth and others added 2 commits September 29, 2023 08:45
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
lib/internal/modules/run_main.js Show resolved Hide resolved
test/parallel/test-process-argv1.mjs Outdated Show resolved Hide resolved
test/parallel/test-process-argv1.mjs Outdated Show resolved Hide resolved
@joyeecheung
Copy link
Member

joyeecheung commented Sep 29, 2023

Thinking about the overall concept, I think it would probably make more sense to introduce something like process.entryPoint instead. argv[1] in general can be a pretty ambiguous concept especially in SEA/embedded environments (process.argv0 isn't too great compared to process.execPath either).

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@GeoffreyBooth
Copy link
Member Author

Thinking about the overall concept, I think it would probably make more sense to introduce something like process.entryPoint instead. argv[1] in general can be a pretty ambiguous concept especially in SEA/embedded environments (process.argv0 isn’t too great compared to process.execPath either).

Maybe we can do both? It feels wrong to have process.argv0 without process.argv1, when both get reassigned on startup. We can additionally add process.entryPoint like we have process.execPath.

@joyeecheung
Copy link
Member

joyeecheung commented Sep 29, 2023

I don't think we really need process.argv1 though, as mentioned before, it would be a weird concept in certain environments, also probably weird in node inspect, and if we want to add other node some-command in the future it would be even weirder. I would prefer that we only have a process.entryPoint or process.originalEntryPoint (to emphasize that it's not expanded) that works across most environments, and not leave a quirky process.argv1 around.

@aduh95
Copy link
Contributor

aduh95 commented Sep 29, 2023

Whatever what name we decide to go with, it'd be very useful to find a way to expose the info to loader thread.

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Sep 29, 2023

I don’t think we really need process.argv1 though, as mentioned before, it would be a weird concept in certain environments, also probably weird in node inspect, and if we want to add other node some-command in the future it would be even weirder. I would prefer that we only have a process.entryPoint or process.originalEntryPoint (to emphasize that it’s not expanded) that works across most environments, and not leave a quirky process.argv1 around.

So it’s the new value of argv[1] that is most useful; I’d much rather have /var/www/app/entry.js than ./entry.js, i.e. the resolved path rather than the raw input. So we could make a process.entryPoint, but it should be the resolved path (or URL?) and then there’s still no way to recover the pre-reassignment argv[1] value. So then I guess I could make process.originalEntryPoint but it almost makes more sense to call it argv1 since it’s the raw input from argv[1]. Or maybe we just keep doing without a way to recover the actual first argument passed to node. We’ve gotten this far without providing it.

@Qard
Copy link
Member

Qard commented Sep 29, 2023

I'd rather not create a process.argv1. Actually, I feel like process.argv0 should not exist either. Their purpose is not clear from the name and it seems to have little benefit over just doing process.argv[n].

We should be explicit about what we're exposing and why so I feel it should have a better name and, yes, resolving first is probably ideal, for consistency.

@GeoffreyBooth
Copy link
Member Author

Their purpose is not clear from the name and it seems to have little benefit over just doing process.argv[n].

It exists because process.argv0 and process.argv[0] have different values. The latter is the resolved path to the executable, the former is the input that got resolved.

I don’t know what the use cases are for having the input before it gets resolved, and that’s a good argument for not needing process.argv1 either. In my case I was working on implementing --entry-url, and I needed to preserve the original input for when it’s a non-file URL. Since process.argv[1] is a file path, and non-file: URLs can’t be resolved into a file path, I needed some way to store the URL input separate from the file path output. But --entry-url can be achieved in other ways, and doesn’t need to expose process.argv1 as part of its implementation.

@Qard
Copy link
Member

Qard commented Sep 29, 2023

Yes, I'm aware of what argv0 does, but the purpose separate from execPath is confusing. If there's an actual benefit to having that it probably should have a more useful name than argv0. Similarly here, if we want to expose the original contents of argv[0] in some way we should try to be more explicit about the naming so the purpose of it is more clear.

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Oct 1, 2023

I opened this PR because I was surprised to see that we had process.argv0 and not process.argv1; if we create a variable to preserve the pre-replacement value of argv[0], it seems to follow that since we also replace argv[1] we should likewise have a process.argv1. What I get out of this thread is a lot of regret that process.argv0 exists, that it was perhaps a bad idea in the first place, etc.; but since it does exist, and I assume we aren’t removing it anytime soon, is there a reason not to complete the pattern with process.argv1? It feels like a bug that we don’t provide process.argv1.

If we want to add additional properties like process.entryPoint like we also have process.execPath, I’m totally fine with those too; I would perhaps wait until after --entry-url lands in some form so that we know how to deal with URLs and/or paths for those, so I think that can come in a follow-up.

@joyeecheung
Copy link
Member

I don't think having process.argv0 results in having to have process.argv1, because by that logic, why would we not have process.argv2, process.argv3, ...? If we really need an additional property for unmodified arguments, process.originalArgv as an array would be better. Even then I doubt it would be any more useful than just having process.entryPoint due to the quirks mentioned.

@GeoffreyBooth
Copy link
Member Author

by that logic, why would we not have process.argv2, process.argv3

Because Node doesn’t reassign process.argv[2] or process.argv[3] on startup.

If we really need an additional property for unmodified arguments, process.originalArgv as an array would be better.

Agreed! But what we have is process.argv0. It may have been a poor choice, but it’s the choice that was made, so why should it exist and process.argv1 should not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Issues and PRs related to the Node.js command line interface. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants