pkgjs / parseargs

Polyfill of `util.parseArgs()`
Apache License 2.0
121 stars 9 forks source link

Second argument (first in `process.argv`) is always ignored. #146

Closed SpacingBat3 closed 1 year ago

SpacingBat3 commented 1 year ago

Current implementation of @pkgjs/parseargs assumes this to be always correct:

https://github.com/pkgjs/parseargs/blob/67749086a6cb216a4896283de200b1f36646afdd/index.js#L69-L70

However, in some enviroments, either script is compiled to the executable like in case of pkg-packaged scripts or there's node binary (e.g. electron or any other web app framework using Node) that calls the script directly, without any need of passing an additional argument for a path to the script. Moreover, I think path could be passed after flags rather than before, which would also cause some inconstancy between the polyfill and native API.

Maybe the code should be prepared for those cases as well?

ljharb commented 1 year ago

Such a compiler should probably be wrapping or building modules such that process.argv has a fictional argument added - otherwise all code that assumes “what node always does” would break.

SpacingBat3 commented 1 year ago

@ljharb thank you for the reply, I've actually verified the polyfill behaves exactly the same as Node (not sure why I expected it to work better than this polyfill in terms of the logic) and such compilance is what I think people should care about. That being said, I feel like parseArgs will still change, considering Electron already breaks with the default logic of the args polyfill.

I'll close this issue for now. It might be useful to reopen it after some time, given how many Node.js-based frameworks and tools are there that actually don't care about Node's argv structure.

shadowspawn commented 1 year ago

If your runtime environment follows different conventions than node does with process.argv, then the intended approach is to prepare the arguments yourself and pass them in the configuration:

const result = parseArgs({ args: process.argv.slice(1), strict: false });

The arguments to be parsed were originally a separate parameter, but the normal case is they can be auto-detected so after some discussion they were moved into the configuration.

(For interest, there was explicit support for detecting Electron in the initial submission to the node project, but that got dropped during the pull request to node. Likewise, there was also a suggestion for a process.mainArgs to consult instead of process.argv, still mentioned in the README.)

SpacingBat3 commented 1 year ago

If your runtime environment follows different conventions than node does with process.argv, then the intended approach is to prepare the arguments yourself and pass them in the configuration:

const result = parseArgs({ args: process.argv.slice(1), strict: false });

Yeah, that's what I want to do for now. I just thought Node will take care of the argv better, given how default args value were documented:

(…) Default: process.argv with execPath and filename removed.

This is very non-intuitive description of what's done for those who use Node-based frameworks or mess with argv on their own for some reason and expect that this still would work as it should be. It's not a problem on this project's side so I feel OK to leave it as it is.

shadowspawn commented 1 year ago

Oh right, makes it sound smarter than it is! Thanks for clarifying source of confusion.