Current issue/consideration:

Node.js will spit back a non-zero exit code (9) if any non-node switches are appended to the command line, which is the case by default for some switches like --allow-file-access-from-files and --enable-avfoundation set in atom_main_delegate.

The Node.js flags passed will still work and be correctly set, but this makes me think that a more robust approach might instead be to safelist allowed flags instead of skipping a preset list of disallowed flags, which would ameliorate this issue entirely. What do you all think?

What is the difference between this and the NODE_OPTIONS support we already provide ? AFAIU you can pass these command line params through options https://nodejs.org/api/cli.html#cli_node_options_options ?

I am slightly not okay with maintaining any kind of blacklist for volatile structures, we tried to do that for chromium in the past but quickly proved inefficient #13039.

Also any reason apps want this for production environments ? It would also simply not maintaining a list if its just for dev environment.

At the moment, we introduce a lot of extra patch surface by exposing DebugOptions, as we do currently do a small amount of parsing cli flags in this manner for InspectorAgent initialization. As ux goes - passing flags in this manner is a lot more user-friendly and is most likely what users expect, and this will, more importantly allow us to remove a notable amount of patch surface (Node.js will not extern DebugOptions, whereas the patch in this PR has already been approved upstream) and simplify the way we're passing settings to node for the InspectorAgent, a process i've already started here.

I would also be in favor of moving away from a blacklist approach and into a whitelist approach based on concerns you outlined above from a general safety perspective.

What is the difference between this and the NODE_OPTIONS support we already provide ? AFAIU you can pass these command line params through options https://nodejs.org/api/cli.html#cli_node_options_options ?

I still don't understand why this is needed over NODE_OPTIONS.

I think we can agree inspector related flags are special case and it wouldn't be hard to handle them separately. But I don't see why we have to bring in parsing the whole set of node cli switches support when there is already another way to achieve it NODE_OPTIONS ? if there are flags that aren't allowed in this mode users should get that fixed in Node.js and not through us.

Node.js will not extern DebugOptions, whereas the patch in this PR has already been approved upstream

If the aim is to reduce the patch around DebugOptions, we can handle only the cli arguments related to inspector through this new method. I am still -1 on handling all node cli options through this new method.

I'd be happy to adapt this PR to just allow the set of DebugOptions-specific options and then we could perhaps open the wider conversation of what we want to eventually enable for users, be it flags, NODE_OPTIONS, or both?

I would say that i personally feel NODE_OPTIONS was added as a shortest-path approach to enable handling these options at all, but is less ergonomic/usable an approach in contrast to being able to pass those selfsame options via bespoke flags

I'd be happy to adapt this PR to just allow the set of DebugOptions-specific options and then we could perhaps open the wider conversation of what we want to eventually enable for users, be it flags, NODE_OPTIONS, or both?

Yup that would be great, thanks!

I would say that i personally feel NODE_OPTIONS was added as a shortest-path approach to enable handling these options at all, but is less ergonomic/usable an approach in contrast to being able to pass those selfsame options via bespoke flags

True I agree with that as well but that comes to our personal choice, what I am trying to avoid is double parsing stuff, we already maintain a code path with blacklisted options for NODE_OPTIONS which I am not so happy about, I am declined to add another layer on top of that. Also node gave this feature support despite cli because its easier to pass across environments to child processes, so I think its sufficient.

The primary goal here is to eliminate the patches around DebugOptions for which maintaining a whitelist of node inspector related flags are much easier and solves the issue at hand rather than maintaining an ever growing list of all node cli options.

I'll take a look at these test failures soon but as soon as that's handled this should be g2g.

I'm adding wip tag since there are pending works (fixing tests) in this PR.

@zcbenz thanks - i should be able to get back to this soon!

@codebytere rebasing this against master should resolve the linux failures

Release Notes Persisted

Enable native Electron handling and passing of Node.js cli options.