feat: allow repl pause on exception state to be set from env

[iloveitaly]

Right now, it's not possible to configure the debugger to pause on uncaught exceptions
without manually configuring the pause state on each run. This change would allow the
pause state to be configured via a shell env variable to allow for faster CLI-based node
debugging.

I'm not sure if there's a reason there aren't more configuration options for node inspect,
so I wanted to submit this PR for discussion before writing tests and updating documentation.

[ryanstout]

+1 thanks!

[Trott]

I know there's usually resistance to adding environment variables even moreso than CLI flags, but I'm not sure there's a better route to enable this feature....

@nodejs/post-mortem (Is that even the right group to ping for the CLI debugger?)

[aduh95]

We would want the env variable to be documented along side the other env variables Node.js supports.

[Trott]

We would want the env variable to be documented along side the other env variables Node.js supports.

True. The OP acknowledged that:

I'm not sure if there's a reason there aren't more configuration options for node inspect,
so I wanted to submit this PR for discussion before writing tests and updating documentation.

Maybe this should be in draft mode.

Anyway, I'm trying to remember who has expressed something at least vaguely like "please, no more environment variables" opinions in the past. Maybe it was @cjihrig?

[cjihrig]

Anyway, I'm trying to remember who has expressed something at least vaguely like "please, no more environment variables" opinions in the past. Maybe it was @cjihrig?

I don't think it was me - at least not originally. IIRC, the agreed upon way to move forward recently has been adding CLI flags and passing those via the NODE_OPTIONS environment variable.

[GeoffreyBooth]

Since you can always set flags via the NODE_OPTIONS variable, like NODE_OPTIONS=--inspect, adding flags is considered preferable as then Node can be configured via either flags or an environment variable. Can this PR be refactored to use a flag instead?

If the new option applies only to node inspect, it could be a flag that comes after that like node inspect --pause-on-uncaught-exceptions or whatever, where this flag exists for node inspect but not for node generally. This wouldn’t be settable via environment variable, though, but since node inspect is interactive I’m not sure its options need to be configured via environment variable?

[Trott]

If the new option applies only to node inspect, it could be a flag that comes after that like node inspect --pause-on-uncaught-exceptions or whatever, where this flag exists for node inspect but not for node generally. This wouldn’t be settable via environment variable, though, but since node inspect is interactive I’m not sure its options need to be configured via environment variable?

While that might present challenges for the NODE_OPTIONS utility here, it might make sense if we think there are going to be a lot of these flags/settings.

And on the other hand, there already is the NODE_INSPECT_RESUME_ON_START environment variable, so that could argue for continuing down that path, I guess.

Since this is for a developer CLI convenience, I think we'd want something that they could set once and never have to think about again. That implies an environment variable, and it also implies one that isn't NODE_OPTIONS since we wouldn't want to include things that could interfere with normal Node.js operations, just CLI debugging.

Or on the other hand, as Geoffrey points out, it could just be a flag and developers could set up a shell alias or whatever.

I should probably collect my thoughts and come up with a recommendation rather than all this listing out of pros and cons.....

[bnoordhuis]

And on the other hand, there already is the NODE_INSPECT_RESUME_ON_START environment variable, so that could argue for continuing down that path, I guess.

To counter-argue: that was introduced when node-inspect was moved into core, i.e., a third-party indiscretion.

FWIW, I was probably the one who first pushed back against adding yet more environment variables. They're essentially ambient state and that's something to avoid as much as possible.

[Trott]

FWIW, I was probably the one who first pushed back against adding yet more environment variables. They're essentially ambient state and that's something to avoid as much as possible.

Works for me. I'm happy to be on board with "use CLI flags and set them in NODE_OPTIONS" as a general rule.

Honestly, if we were designing this particularly thing (CLI debugging settings) from scratch, I wonder if the approach we'd want is to read some kind of config file.

[iloveitaly]

but I'm not sure there's a better route to enable this feature....

The other idea I had was using something like ~/.node_inspectrc with json or javascript to configure these options:

{
  "pauseOnException": "uncaught"
  "port": 0,
  "inspectResumeOnStart": true
}

The interesting thing about the javascript model here is you could add a configuration hook to do all sorts of customization (like python, ruby, etc) have:

module.exports = function(repl, debugger, etc) {
  repl.shortcuts.push(...)
}

Can this PR be refactored to use a flag instead?

Yes, definitely!

If the new option applies only to node inspect, it could be a flag that comes after that like node inspect --pause-on-uncaught-exceptions or whatever, where this flag exists for node inspect but not for node generally.

I'm with this approach, and maybe we could add a NODE_INSPECT_OPTIONS env var where everything could be configured.

Needing to have a ton of options passed to the CLI right now degrades the UX of node inspect in general. My ideal experience is being to set all of my config via env var (with something like direnv) so I can just node inspect something.ts to get debugging right away.

---

In summary, it sounds like what is being proposed is:

1. Use -- command line arguments instead of environment variables
2. Ensure the arguments are specific to node inspect, which in turn means the argument parser should be upgraded to be more flexible beyond the current regex approach

I would first submit a PR to refactor the argument parser, and then update this PR to use --pause-on-exception-state

What I'd like to propose is in addition to the above is:

1. Initially, add support for a NODE_INSPECT_OPTIONS env var which could be used to replace the bespoke NODE_INSPECT_RESUME_ON_START var (and any other similar vars if they exist)
2. In the future, a ~/.nodeinspectrc file, with the name, format (json/javascript/both), and available options to be debated at a later time.

What do folks think?

[GeoffreyBooth]

What I'd like to propose is in addition to the above is:

1. Initially, add support for a NODE_INSPECT_OPTIONS env var which could be used to replace the bespoke NODE_INSPECT_RESUME_ON_START var (and any other similar vars if they exist)
2. In the future, a ~/.nodeinspectrc file, with the name, format (json/javascript/both), and available options to be debated at a later time.

I like the first one. I think ideally, we would add support for node inspect-only flags like node inspect --pause-on-exception-state where this flag --pause-on-exception-state works for node inspect but not for node. Then yes, we also create NODE_INSPECT_OPTIONS where it can follow the pattern of NODE_OPTIONS so like NODE_INSPECT_OPTIONS=--pause-on-exception-state.

Re ~/.nodeinspectrc, there are already a few open issues about supporting configuring Node via files. I would prefer to adopt a general solution for supporting all of Node’s configuration options via a file, rather than a config file that works only for node inspect. One approach already discussed would be to give Node support for .env files like Bun has; that allows for both configuring Node (if NODE_OPTIONS and/or NODE_INSPECT_OPTIONS are defined in the file) as well as setting whatever other environment variables the user would want defined for the application. Besides being very powerful, this approach seems likely to be the easiest solution to implement, as parsing a .env file within the C++ parser is probably more straightforward than JSON or running JavaScript, and we can always add support for JSON or other formats later in addition.

[iloveitaly]

Update here for @GeoffreyBooth, @Trott, and team!

• I've added support for NODE_INSPECT_OPTIONS although I've not added support for --port and other options there. Curious if you think this is needed right away, or we can do this in a separate PR. I'm wary of making breaking changes (even small ones)
• Added support for --pause-on-exception-state and --inspect-resume-on-start

This is working great on my better-node-inspect library, but there's still work left to do here:

• Add tests, if applicable (any points here?)
• Run more manual tests

Wanted to post here and get some initial feedback on the approach before I go farther.

[Trott]

• Add tests, if applicable (any points here?)

Existing tests for the CLI debugger are in test/parallel/test-debugger* and (for tests that use an explicit port number or otherwise can't be run reliably while other tests are running) test/sequential/test-debugger*.

The helper library for CLI debugger tests is documented in test/common/README.md which can also be accessed at https://github.com/nodejs/node/tree/main/test/common#debugger-module. (Looking at it now, the debugger helper stuff can use considerably more documentation. But hopefully the function names are clear enough along with the existing tests for reference.)

For non-debugger-specific stuff about tests, there is a guide in doc/contributing/writing-tests.md which can also be accessed at https://github.com/nodejs/node/blob/main/doc/contributing/writing-tests.md.

You can also ask questions about writing tests for Node.js core in the #nodejs-core channel on the OpenJS Slack: https://openjs-foundation.slack.com/archives/C019Y2T6STH

[Trott]

Existing tests for the CLI debugger are in test/parallel/test-debugger* and (for tests that use an explicit port number or otherwise can't be run reliably while other tests are running) test/sequential/test-debugger*.

Oh, it appears that there are also a small number of them in test/parallel/test-inspect*. Unless there's a reason for the discrepancy, it would probably be a good idea to get them all the CLI debugger tests to be named consistently as either test-debugger* or test-inspect*.