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

Programmatically expose whether a cli flag takes an argument or not #54144

Open
nicolo-ribaudo opened this issue Jul 31, 2024 · 4 comments
Open
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@nicolo-ribaudo
Copy link
Contributor

What is the problem this feature will solve?

I am trying to build a wrapper around the Node.js CLI, and for that I need to know which flags are meant to be passed to Node.js and which one should be passed to the script.

I was hoping that process.allowedNodeEnvironmentFlags would help with this, but unfortunately it's not enough. Consider this two commands:

node --some-flag --some-other-flag foo bar --baz

And assume that process.allowedNodeEnvironmentFlags includes --some-flag and --some-other-flag. What are the arguments being passed to node, and what to the script? which script?

There are two possible answers:

  • --some-other-flag takes a value, so --some-flag --some-other-flag foo are for node, the script is bar, and --baz is in process.argv;
  • --some-other-flag does not take a value, so --some-flag --some-other-flag are for node, the script is foo, and bar --baz is in process.argv.

What is the feature you are proposing to solve the problem?

Make process.allowedNodeEnvironmentFlags a map with boolean values telling whether the flag takes a value or not. The logic to use it would then become:

function splitArgs(allArgs) {
  let nodeArgs = [];
  let scriptName;
  let scriptArgs = [];

  let i = 0;
  while (i < allArgs.length && allArgs[i][0] === "-") {
    nodeArgs.push(allArgs[i]);
    if (process.allowedNodeEnvironmentFlags.get(allArgs[i])) nodeArgs.push(allArgs[++i]);
    i++;
  } 
  scriptName = allArgs[i++];
  while (i < allArgs.length) scriptArgs.push(allArgs[i++]);

  return { nodeArgs, scriptName, scriptArgs };
}

For backward compatibility, this map should still have a no-op .add method.

What alternatives have you considered?

I can hard-code the list. However, this means that for every new release I have to check if there is any change in the supported flags, and I have to maintain multiple lists one per version.

The maintenance cost for Node.js is much lower, given that new flags already have to be added to process.allowedNodeEnvironmentFlags anyway.

@nicolo-ribaudo nicolo-ribaudo added the feature request Issues that request new features to be added to Node.js. label Jul 31, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Jul 31, 2024
@targos
Copy link
Member

targos commented Jul 31, 2024

Another issue with process.allowedNodeEnvironmentFlags for your use case is that it doesn't include all flags supported by Node.js, but only those which can be set in NODE_OPTIONS

@joyeecheung
Copy link
Member

joyeecheung commented Aug 1, 2024

We internally currently have require('internal/options').getCLIOptionsInfo() which generates something like this to the JS land

[Object: null prototype] {
  options: SafeMap(182) [Map] {
    '--disable-wasm-trap-handler' => [Object: null prototype] {
      helpText: 'Disable trap-handler-based WebAssembly bound checks. V8 will insert inline bound checks when compiling WebAssembly which may slow down performance.',
      envVarSettings: 0,
      type: 2,
      defaultIsTrue: false
    },
    '--track-heap-objects' => [Object: null prototype] {
      helpText: 'track heap object allocations for heap snapshots',
      envVarSettings: 0,
      type: 2,
      defaultIsTrue: false
    },
    ...
  },
  aliases: SafeMap(24) [Map] {
    '--debug-port' => [ '--inspect-port' ],
    '-p' => [ '--print' ],
    '--loader' => [ '--experimental-loader' ],
   ...
}

I think it would be a good idea just expose some APIs to 'node:util' to return a copy/subset of the options map and maybe a different API to return a copy of the aliases too. Some caveats come to mind:

  1. options contain some "hidden" options that are only there to indicate flag implications (this has something to do with how the option parser works internally), they are not meaningful for users and need to be left out. There are also a handful of v8 options that are there for a similar reason and should be filtered out too (this is only a very small subset that serves NODE_OPTIONS allowlisting, IIRC the V8 team was against exposing a list of V8 options in the process.allowedNodeEnvironmentFlags API because the V8 flags are very unstable and they were worried about the maintenance burden, see process: add allowedNodeEnvironmentFlags property #19335).
  2. defaultIsTrue is an ad-hoc thing for boolean options, there are some options with alternative default values that may be set programically in the C++ land and it may be harder to ensure keeping them represented in JS land. The default values should be left out from a public API for now until we refactor this properly somehow to make the default values more deterministic.
  3. The returned values should be copies to avoid tampering (although we don't currently use this internal API for side-effect-less uses, this internal API was split out to only serve generating texts for --help and to build process.allowedNodeEnvironmentFlags, but I suspect there might be other future use of this, so copies would be safer).
  4. I am not sure whether help text should be exposed, they do seem useful but we certainly don't want changes to them to be semver-major. Maybe we should just label the output of the entire API to be out of semver - you can only count on its format, but don't count on its output to be non-breaking acroos the minor/patch releases.

@joyeecheung
Copy link
Member

Actually now I looked at it it seems strange why we didn't just expose something similar to the JS land but chose to provide a set that's very specific to the ones allowed in NODE_OPTIONS in the form of process.allowedNodeEnvironmentFlags. Although the internal API for this was only split out recently in #52451 but it could've been split all along. Is there a specific reason why we chose to provide a more restricted API? I can't seem to find in #19335 maybe @boneskull still remembers (though it's from many years ago)

@boneskull
Copy link
Contributor

It's for NODE_OPTIONS, specifically.

Because if we're talking about env vars, they all have "values" (even if that value is '1'), so it didn't matter.

@nicolo-ribaudo Take a look at e.g. c8 or mocha, both of which do something like what you're doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
Status: Awaiting Triage
Development

No branches or pull requests

4 participants