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

enhancement: provide list of allowed flags of current executable #17740

Closed
boneskull opened this issue Dec 18, 2017 · 22 comments
Closed

enhancement: provide list of allowed flags of current executable #17740

boneskull opened this issue Dec 18, 2017 · 22 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@boneskull
Copy link
Contributor

boneskull commented Dec 18, 2017

It'd be useful for Mocha and other certain CLI applications to have a list of supported flags of the current process available.

Many users which to execute a CLI app like mocha with Node.js flags, e.g. --experimental-modules. Mocha spawns a node process with any requested flags in place, and Mocha loads/runs JavaScript files from within this child process (there may be better ways to do this, but that's not what this issue is about; please put your ideas in Mocha's issue tracker 😉).

This puts a maintenance burden on Mocha to explicitly add support for new Node.js flags (I can provide a list of PRs over the years, but this is basically a 1:1 relationship between new Node.js flags and pull requests).

These flags appear in mocha --help. Mocha is unable to remove any flags unless it maintained a lookup of Node.js-version-to-allowed-flags itself (FWIW, I'm unsure if Node.js has ever actually removed a flag).

(Mocha could simply support something like --node-flags='--harmony --trace-warnings' option, but this a little too user-hostile for my taste, and doesn't solve the problem of listing the flags.)

Flags listed under --v8-options are not shown in mocha --help, because Mocha can blindly pass through any flag matching ^--v8-.*. In the context of this issue, I'm not proposing v8-specific flags are listed.


These two issues (adding new flags, removing old ones) could be mitigated (going forward) if Node.js provided information on allowed flags in its current executable via an Array in process. For example:

> process.allowedFlags
[
  {
    name: 'v', 
    alias: 'version', 
    description: 'print Node.js version'
  },
  {
    name: 'napi-modules', 
    description: 'load N-API modules (no-op - option kept for compatibility)'
  },
  {
    name: 'trace-warnings', 
    description: 'show stack traces on process warnings'
  }
  // etc etc
]

Mocha and other CLI apps like it could consume this information and use it to allow/disallow flags as well as print the information in a "help" page.

Reasonable?

@bnoordhuis
Copy link
Member

Mocha could simply support something like --node-flags='--harmony --trace-warnings' option, but this a little too user-hostile for my taste.

Why is that user-hostile? I ask because...

Flags listed under --v8-options are not shown in mocha --help, because Mocha can blindly pass through any flag matching ^--v8-.*.

...we can't list V8 flags (node.js doesn't know them) and that makes process.cliFlags or whatever it's called decidedly less useful, IMO.

There's also the question of how (or if) it should interact with the NODE_OPTIONS environment variable.

FWIW, I'm unsure if Node.js has ever actually removed a flag

Yes, although not recently, I think.

@boneskull
Copy link
Contributor Author

boneskull commented Dec 18, 2017

...we can't list V8 flags (node.js doesn't know them) and that makes process.cliFlags or whatever it's called decidedly less useful, IMO.

You are entitled to your opinion, but this will reduce the number of PRs we have to merge to support new flags to 0.

Mocha could simply support something like --node-flags='--harmony --trace-warnings' option, but this a little too user-hostile for my taste.
Why is that user-hostile? I ask because...

Simply because we can't list them. Since Node.js can't list the v8 flags either, it's not an expectation.

@bnoordhuis
Copy link
Member

You are entitled to your opinion, but this will reduce the number of PRs we have to merge to support new flags to 0.

I most certainly am and that argument doesn't sway me because it means the PRs we have to merge becomes > 0. :-)

Simply because we can't list them.

I don't understand why that is important. node --help and pipe the output?

@boneskull
Copy link
Contributor Author

I don't understand why that is important. node --help and pipe the output?

Are you serious?

@bnoordhuis
Copy link
Member

Yes. Stick to substantial comments, will you?

@boneskull
Copy link
Contributor Author

boneskull commented Dec 18, 2017

Piping node --help to the output would:

  1. add slowness to a simple mocha --help
  2. necessitate a regex or otherwise parsing the output of node --help to remove everything but the options
  3. filter out things that conflict like -v (by regex, probably)
  4. continued maintenance burden if the output of node --help changes and breaks our parsing
  5. the maintenance burden inherent in trying to machine-read something that's not designed to be machine-readable, like node --help.

That's why.

@apapirovski
Copy link
Member

I'm probably missing something super obvious but is there a reason Mocha wouldn't just blacklist its own flags and pass through anything that doesn't match?

I mean, I get that node won't run if invalid flags are passed to it but, strictly speaking, users probably shouldn't be passing in flags that neither mocha nor node want to consume... ?

@boneskull
Copy link
Contributor Author

I mean, I get that node won't run if invalid flags are passed to it but,

This is why; coupled with wanting to list the flags so it's plain to see which flags are invalid and which aren't.

For example:

If you run mocha --butts, which you expected to be a valid flag to node, and it fails, then you are going to run node --butts and see that it was a typo, and --buttz is the correct flag. Then you can go back and run mocha --buttz.

The above is more difficult than it needs to be. mocha --help should list the Node.js flags.

users probably shouldn't be passing in flags that neither mocha nor node want to consume

Users do a lot of things...

@apapirovski
Copy link
Member

apapirovski commented Dec 18, 2017

I suppose this means you wouldn't pass through -- --my-script-flag then? *shrug*

This all seems very convoluted and limiting to me when the solution to the perceived problem is to tell users to check node --help.

@bnoordhuis
Copy link
Member

Okay, I understand a bit better now, although it seems easy to fix by adding one line to mocha --help that says to also check node --help or pipe the output after mocha's own options.

add slowness to a simple mocha --help

Something like 50 extra milliseconds. That's not going to break the bank.

I'm not mordicus opposed if we could fix the V8 options issue but I share Anatoli's sentiment that this seems to be a solution in search of a problem.

@boneskull
Copy link
Contributor Author

@apapirovski This issue is not about whether or not you think that's a real problem or a perceived one. I maintain a module in which thousands of users rely on this behavior and expect it to work. Whether or not you find it helpful, it's helpful to many others.

This issue is about the maintenance burden of supporting it, and how that can be significantly reduced by making this information easily accessible in core.

@boneskull
Copy link
Contributor Author

Something like 50 extra milliseconds

It'll take a bit longer than that on a Raspberry Pi Zero...

@apapirovski
Copy link
Member

This issue is not about whether or not you think that's a real problem or a perceived one. I maintain a module in which thousands of users rely on this behavior and expect it to work. Whether or not you find it helpful, it's helpful to many others.

It's not like I'm blocking any potential work being done on this, I'm just opining as to why I don't think this is a good solution to the problem it's supposed to solve. Even if we assume that this is a good solution, how do we deal with - and --? It seems like we're back to having the user-land filter this list and maintain their own blacklist. Also, if options are added that conflict with ones that already exist in mocha then those need to be filtered out based on a blacklist of existing options within mocha.

@jasnell
Copy link
Member

jasnell commented Dec 18, 2017

This would be a bit non-trivial for us to support given the current mechanism in core for handling command line options. If we could refactor that code, then it is conceivable that we could make it near zero maintenance cost for us to provide this kind of API. I do not see it as being unreasonable at all.

@benjamingr
Copy link
Member

I don’t think we should support this given how much work it is to do - but if you make a PR and do it in a way that doesn’t make the code harder to maintain then I’m game.

@bnoordhuis bnoordhuis added the feature request Issues that request new features to be added to Node.js. label Dec 19, 2017
@bnoordhuis
Copy link
Member

I gather from external discussion that mocha is probably going down the --node-<option> path, like it does for V8 options with --v8-<option>.

I'm not aware of other potential users and a lot of work would need to happen to stop it from being a maintenance hassle. Unless someone volunteers to do the work I suggest we close this.

@benjamingr
Copy link
Member

I think we should give it a few days to see if @boneskull or anyone else is interested in pursuing a PR with an approach that would not make the code harder to maintain.

@boneskull
Copy link
Contributor Author

I gather from external discussion that mocha is probably going down the --node- path, like it does for V8 options with --v8-.

News to me...

Considering this issue and previous poor experiences attempting to collaborate with Node core, I won’t be sending a PR.

@Trott
Copy link
Member

Trott commented Dec 19, 2017

Considering this issue and previous poor experiences attempting to collaborate with Node core, I won’t be sending a PR.

Ref: #10473 (comment)

(Providing specific context for the previous comment, that's all. If anyone wants to have further discussion around that, the probable sensible places for it are probably the issue trackers for https://github.com/nodejs/community-committee/ and/or https://github.com/nodejs/TSC. This issue is probably not the place.)

boneskull added a commit to boneskull/node that referenced this issue Aug 23, 2018
`process.allowedNodeEnvironmentFlags` provides an API to validate and list flags as specified in `NODE_OPTIONS` from user code.

Refs: nodejs#17740
Signed-off-by: Christopher Hiller <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this issue Aug 25, 2018
`process.allowedNodeEnvironmentFlags` provides an API to validate and
list flags as specified in `NODE_OPTIONS` from user code.

Refs: nodejs#17740
Signed-off-by: Christopher Hiller <[email protected]>

PR-URL: nodejs#19335
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this issue Aug 28, 2018
`process.allowedNodeEnvironmentFlags` provides an API to validate and
list flags as specified in `NODE_OPTIONS` from user code.

Refs: #17740
Signed-off-by: Christopher Hiller <[email protected]>

PR-URL: #19335
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 3, 2018
`process.allowedNodeEnvironmentFlags` provides an API to validate and
list flags as specified in `NODE_OPTIONS` from user code.

Refs: #17740
Signed-off-by: Christopher Hiller <[email protected]>

PR-URL: #19335
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Sep 6, 2018
`process.allowedNodeEnvironmentFlags` provides an API to validate and
list flags as specified in `NODE_OPTIONS` from user code.

Refs: #17740
Signed-off-by: Christopher Hiller <[email protected]>

PR-URL: #19335
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@richardlau
Copy link
Member

@boneskull Can this be closed now that #19335 landed?

@gireeshpunathil
Copy link
Member

ping @boneskull - is this still outstanding?

@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Closing since this seems fixed.

@BridgeAR BridgeAR closed this as completed Jan 2, 2020
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
None yet
Development

No branches or pull requests

9 participants