Node: Uniform way to trigger debugger on first line

[refack]

Ref: #12364

1. ##### Have --inspect --debug-brk as a uniform way to trigger debug on first line
   
   
   
   1. Restore the --inspect --debug-brk combo to v7+v8 (#12580)
   
   
   2. Does it stay undocumented? Or maybe it should become the primary option and remove --inspect-brk (re: #11441)
      
      (IMHO if we keep it --debug-brk, --inspect-brk will probably never be used)
   
   
   3. Will the __combo__ be valid in node8?
   
   
   4. What do we do with --debug-port / inspect-port?
   
   
   5. In light of these, consider what to mark as deprecated.
   
   
   6. Since v6 and v7 requires the combo --inspect[=port] --debug-brk[=port] is specifying port on both args a valid invocation, and in that case which port wins?
   
   
2. ##### Have --inspect-brk as a uniform way to trigger debug on first line
   
   
   
   1. Keep the --inspect --debug-brk combo in v7 alone (deprecation notice yes/no) (i.e. don't land #12197 in v7)
   
   
   2. port the --inspect-brk alias to v6 (#12615)
      
      [new comment] this will make --inspect-break a feature of recent versions of 6.x, but it can't change the past: versions of 6.x will always exist without this feature, and so will not be debuggable by third-party tooling [without special treatment]
   
   
   3. for v4 it's irrelevant since it's a different protocol and other means of detection and handling is necessary
   
   
3. ##### Help the users adapt to our plan:
   
   
   
   1. Help fix VSCode properly, make it compatible with our current and future plans (/cc @roblourens Ref: Microsoft/vscode-node-debug2#100)
   
   
   2. Make sure JetBrains handle node8 nightlies (/cc @ulitink @segrey youtrack#WEB-26568)
   
   

User feedback

I'm trying to get more feedback from @roblourens and JetBrains, so you could make the best decision.

1. Quote from youtrack#WEB-26568
   

2. Comment from @roblourens https://github.com/nodejs/node/issues/12364#issuecomment-296758944

P.S. at present WebStorm and IDEA based IDEs can't trigger debug in node8 nightlies (nor can VSCode)

[refack]

After hearing from the IDE vendors, IMHO we should forget about --inspect-brk, it won't get adopted, and just keep --debug-brk permanently.
Protocol detection & resolution is done in orthogonal ways anyway.

[sam-github]

I'm OK with keeping --debug-brk permanently as the flag meaning "break-on-first-line" (too bad it wasn't called that originally). It seems analogous (to me) with how node debug starts the inspector in most recent. In which case, we don't have to backport anything, we just start the deprecation process for --inspect-brk (so long, barely had time to know you).

I'm also OK with backporting --inspect-brk.

[gibfahn]

It seems to me that if you ignore the --inspect --debug-brk combo, then the situation becomes much more simple. Going forward we want a flag that means "start the debugger and break on the first line", and we have two options. The pros and cons are:

| Choice | Pros | Cons |
| --- | --- | --- |
| --inspect-brk | --inspect+--inspect-brk makes sense | it's another option to remember, --debug-brk already exists |
| --debug-brk | Old flag, new protocol, just keeps working. Also more intuitive if you don't know or care what the inspector is. | May be confusing that --debug-brk on v6 !== --inspect --debug-brk on v6 and !== --debug-brk on v7 |

Thinking about this further, I'd be +1 for abandoning --inspect-brk and sticking with --debug-brk. By Node 10 or 11, no-one will (hopefully) remember or need to care about the difference between the two protocols, we'll just have the one. And at that point having --debug-brk mean "start a debugger and break" seems like the natural choice.

If we're going to do that then we should not backport --inspect-brk, and we should re-add --debug-brk to master.

_EDIT:_ I'd also say that the number of times I've used the inspector without the brk option is very small, I'd say this is really the default option for a user.

[gibfahn]

I'm OK with keeping --debug-brk permanently as the flag meaning "break-on-first-line" (too bad it wasn't called that originally).

@sam-github the thing is that you're not supposed to do --inspect --debug-brk, you're just supposed to do --debug-brk, which debugs _and_ breaks. So it actually seems pretty well named to me. The --inspect --debug-brk thing is a temporary aberration caused by having two debug protocols in one version of Node.

[refack]

@sam-github the thing is that you're not supposed to do --inspect --debug-brk, you're just supposed to do --debug-brk, which debugs and breaks. So it actually seems pretty well named to me. The --inspect --debug-brk thing is a temporary aberration caused by having two debug protocols in one version of Node.

If you look at the code it's actually syntactic sugar for three operations:

1. Choose protocol
2. Set port
3. Break on first line

I think ideally it would have only been used by users, while vendors used the three explicit args
--inspect --brake-on-first --debugger-port=5599

BTW: should it be "break" of "brake"?

[gibfahn]

I think it's break as in breakpoint.

[sam-github]

@Trott @eugeneo thoughts? doesn't matter to much too me which we do, but we need to do one or the other, allow --debug-brk in all versions, or backport --inspect-break.

[Trott]

@Trott @eugeneo thoughts? doesn't matter to much too me which we do, but we need to do one or the other, allow --debug-brk in all versions, or backport --inspect-break.

I'm happy to defer to the judgment of @nodejs/diagnostics and @nodejs/LTS on this.

[MylesBorins]

I think we should first define the best ux for debugging with a break, and
then work backwards.

[refack]

The IDE vendors are now making it clear a breaking change in args will not be backported:

JetBrains: https://github.com/nodejs/node/issues/12364#issuecomment-296985945

VScode: https://github.com/nodejs/node/issues/12364#issuecomment-296908854

(the 👍s is mine)

Seems we'll have to keep --inspect --debug-brk for the time being. So IMHO --inspect-brk and inspect-port are redundant...

[Fishrock123]

My personal, pedantic vote is --break-on-first-line.

Alternatively, --{debug|inspect}-pause probably make more sense.

... I suppose the ship has already sailed on that though, so maybe we should just keep --debug-brk because it is essentially just adding a debugger; which shares a similar name-ism.

[sam-github]

I'm OK with @refack and @Fishrock123 's suggestion of keeping --debug-brk and --debug-port indefinitely as the documented CLI options.

What about the briefly existing --inspect-brk and --inspect-port, leave them indefinitely, but undocumented? Or should we start the deprecation process (doc deprecate for 8.x, runtime deprecate for 9.x, remove in 10.x)?

[Fishrock123]

I think --debug-port should be done away with in favor of --inspect-port, fwiw.

[roblourens]

I wish we had added --inspect-brk from the beginning - I agree at least that it looks nicer and is consistent with --debug-brk. But I also wonder, in the future when the old protocol is long forgotten, will --inspect be a good name? If someone is looking for an option to "debug", will they discover it? I wonder where it will make sense to add --debug as an alias at some point.

[sam-github]

@Fishrock123 so, you want node debug, --inspect, --inspect-port, and --debug-brk?

[jkrems]

--inspect-brk is the only way to start the mostly-desired debug behavior with one single flag. I really don't think we should remove it. I'd be fine with keeping --debug-brk/--debug-port around as a deprecated features as long as it's only valid in combination with --inspect. I'm not sure why anyone in the "glorious future" would prefer --inspect --debug-brk --debug-port=1234 over --inspect-brk=1234.

[Fishrock123]

No I would rather it all just be inspect but I guess we might have to make a compromise somewhere.

[jasnell]

I would prefer --inspect, --inspect-port and --inspect-brk. We can add --inspect-port and --inspect-brk as aliases of --debug-brk and --debug-port in LTS streams, and --debug-brk and --debug-port as undocumented, deprecated aliases of their --inspect-* counterparts going forward.

[gibfahn]

I'm not sure why anyone in the "glorious future" would prefer --inspect --debug-brk --debug-port=1234 over --inspect-brk=1234.

@jkrems You wouldn't do that, in Node 8 node --debug-brk=1234 would be identical to node --inspect-brk=1235 (which is why we only want to keep one). So the question is --debug-brk or --inspect-brk.

[sam-github]

We can have it all be inspect, we just have to backport --inspect-brk, and support --debug-brk going forward for a while. Maybe the CTC needs to hash this out f2f.

[jkrems]

@gibfahn But then a tool that passes only --debug-brk={myPort} would see widely different behavior between {any other version of node} and node 8. And there wouldn't be any good indication for why it broke. Having the same set of command line flags enable entirely different protocols is a bit too evil for my taste.

[refack]

I'm not sure why anyone in the "glorious future" would prefer --inspect --debug-brk --debug-port=1234 over --inspect-brk=1234

@jkrems I think we have two use-cases:

1. Automated tools: who might prefer explicit 3 part arg:
   1.protocol - --inspect/debug
   2.should-stop-on-first yes/no - --debug-brk (if we could give it a better name we should)
   3.which port --debug-port=1234
   IMHO this combination can be formed in a simpler if-else process.
2. CLI user - who probably prefer the shortcut --inspect-brk=1234, can make complex decisions and adapt to new syntax.

[refack]

As the one who triggered this whole mess, and got convinced by the Vendors I want to remind you:

At present not JetBrains' tools nor VSCode or VS2017 can debug node8 nightlies. And they say they will not backport a fix to old version.

I'm still trying to get feedback from the VS team.

[jkrems]

@refack I'm +100 for adding back support for --inspect --debug-brk in node 8. This exact problem also bit ourselves in node-inspect. I was only responding to "should we drop --inspect-brk?" / "should --debug-brk start the v8 inspector protocol?".

[refack]

@jkrems I think that's the thing we're stuck with (--inspect --debug-brk support). Now we're bikeshading about the future...
My estimation is that the vendors will not have any motivation to change to a synonymous --inspect-brk, they will not backport any fixes. So I'm just asking why have two sets of switches?
I agree --inspect-brk makes more sense if we think in the context of the whole inspect/debug issue. But for the vendors it's a solved problem, they already forgot it...
IMHO from now on node just has a debugger and we want it to stop on the first line...

I'm not in the CTC and I defer to their judgment.

[billti]

Thanks @refack

For the Node Tools for Visual Studio, we recently committed a change (https://github.com/Microsoft/nodejstools/commit/85b1c13cc01129a479a7f632a4b50f989032f153) to use --inspect-brk if we detect Node.js 8 or later.

Per the above discussion, the discrepancy in flags across 7 & 8 was a little confusing, and only caught late, as we had been testing the --inspect protocol with recent Node.js 7 builds using --debug-brk.

That said, I'm in agreement with @jkrems above that having the same switch enable two very different protocols on different Node.js versions seems undesirable. If --inspect and --debug are going to remain quite distinct across versions 6 and 8, then it seems consistent that the -brk derivatives should follow suite.

[refack]

Thank you @billti for the feedback.
May I asked two follow up questions:

1. I deduce that you are already doing "version detection", right? In that case how do you it when the user chooses to wrapper scripts as executable (like npm.cmd, mocha or babel)?
2. Will this change be backported to VS015?

[billti]

For #1, we try to locate the node.exe via a few fallback methods if a path to node.exe is not provided in the project settings - see https://github.com/Microsoft/nodejstools/blob/master/Nodejs/Product/Nodejs/Nodejs.cs#L66 .

For #2, our Chrome Debug Protocol support depends on some changes that are only in VS 2017, so VS 2015 will only support debugging < Node.js 8. (On < VS 2017 you get an error message that this version of Node.js isn't support -https://github.com/Microsoft/nodejstools/blob/master/Nodejs/Product/Nodejs/Nodejs.cs#L152 )

[jkrems]

@billti For 2.: That wouldn't be the case if we'd make node --inspect --debug-brk work in node 7 (or even 6)? Or is VS2015 restricted to the old protocol, period?

[billti]

It's not a question of command-line switches, it's a question of protocol support. If Node.js 8+ only supports the Chrome Debug Protocol, then it will only be supported from VS 2017 onward.

[refack]

[offtopic]
It always makes my smile (and proud) that that VScode is electron and that VS uses nodejs:

No disrespect intended in the least, both are very fine products that I (the rare Windows bird around here) use them allot.

[refack]

@billti @roblourens @prigara we would like to hear your latest opinions on two things:

1. Is there consensus that --inspect --debug-brk is necessary for compatibility with existing tools?
2. What is your preference for future syntax?
   
   
   
   1. adopt single argument --inspect-brk[=[host:]port], and backport it?
   
   
   2. adopt the explicit three argument combo (protocol, port, is-to-break)
      
      --inspect --debug-port=HOST:XXX --debug-brk?
   
   
   3. keep the 2 argument combo --inspect --debug-brk for the foreseeable future?
   
   

[jkrems]

I thought the general agreement was:

1. We need to keep --inspect --debug-brk working for the time being.
2. For better usability & less confusion --inspect-brk is still a good thing and - imo - should definitely be ported to node 6.

This means future tools (after node 6 phased out) won't have to deal with any "why are halve of the flags called --inspect and the other half --debug", users get CLI flags as nice & succinct as the original --debug/--debug-brk, and older IDEs will work against node 8.

[refack]

@jkrems I also think there is wide agreement on 1
As for 2 I have an assumption the vendors will not adopt --inspect-brk, that's why I asked them.
As for individual CLI users, I have no strong opinion, just the preference that we have a single set of __well-documented__ arguments (so we might need to yield with the vendors 🤷)

[jkrems]

@refack The thing is: node --inspect-brk is a lot less annoying to type than node --inspect --debug-brk. I don't quite get why we would "willfully" regress on the CLI usability, especially given that on current master node --inspect-brk already works. I get keeping the --debug-brk alias around. But not the other part.

P.S.: Even if today's vendors won't use --inspect-brk, in 1 or 2 years different people will build different tools targeting different (read: newer) versions of node. And I'd be surprised if they pick --inspect --debug-brk over --inspect-brk if they have the choice.

[roblourens]

I agree with @jkrems. As long as --inspect --debug-brk works for the foreseeable future, I'm happy. --inspect-brk seems good to keep as well.

[refack]

I kind of agree.
As I see it, the message should be is: for v6 & v7 we had to fast transition our debug protocol. Sorry for confusing our users with new names. node8 has only one debugger and only one (official) way to trigger it (undocumented backward compatibility aliases are a "secret" necessary evil).
What I hope for node8 is to make --inspect-brk == --debug-brk since there is no other debugger.

My personal preference is to make the debug* variants the official variants (IMHO the inspect moniker was as internal name that got leaked to userland).

P.S. voice your opinion on our internal issue #12768

[jkrems]

As pointed out elsewhere in the thread, going back to --debug (or --debug-brk w/o --inspect) anytime soon is pretty evil. The same command line option combination shouldn't trigger 2 completely different protocols across different versions of node. Especially when we have node 6 and 7 where the two protocols coexist.

--inspect* is for the "V8 inspector protocol". --debug* is for the "V8 debugging protocol". There are good reasons to use different flags for different protocols. --inspect --debug-brk is an unfortunate weirdness for historical reasons but we should try to reduce confusion for future releases.

[refack]

I want to reduce bikeshedding...
IMHO The __main__ function of --inspect* and --debug* is to start a debugger.
inspect and debug are different protocols, and that's secondary. Similar to --inspect:9999 and --nspect:1234 listen to different ports.
@jkrems if you still strongly disagree, I yield (pending words from the vendors).

I think we can all agree that the docs and node --help should only display one set (or something with [deprecated] as necessary).

[jkrems]

I think we can all agree that the docs and node --help should only display one set (or something with [deprecated] as necessary).

Yes, those official parameters are --inspect* for the inspect protocol and, for older versions of node, --debug* for V8's old protocol. For versions that support both, node --help would display both. For versions that only support one, it would display one. If somebody explicitly passes --xyz to start one of the protocols, they want to start one specific protocol that their client supports. Otherwise they would click a button in their IDE which in turn would pass specific flags to expose the protocol it expects.

For me the fact that we'll have to keep --inspect --debug-brk around to support "early" adopters of the inspector protocol is the internal detail that we shouldn't leak into our docs & --help output.

[refack]

@Fishrock123 during the transition when both protocols were avalible, all vendors had a version update that was compatible with both. There was no back-porting :(

As mentioned above by all vendors, current versions will not be able to debug node8, and they firmly state that they will not backport. So users will have to upgrade/buy a new IDE

[refack]

All current versions are compatible with both protocols

[Fishrock123]

Cross-posting from: https://github.com/nodejs/node/pull/12580#issuecomment-300542068

-1, I don't really see the point in keeping it when --debug-brk was going to be removed in a major.

I would be for having it print out a notice to use --inspect-brk though.

---

the vendors can't/won't backport --inspect-brk so all current IDEs will not be albe to debug node8

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

[refack]

I'll try to sum:

1. current versions of IDEs work with both protocols
2. vendors have been exclusively using --inspect --debug-brk to trigger inspector
3. if we don't restore alias current IDEs will not work with node8
4. Give vendors a year to shift

[refack]

the vendors can't/won't backport --inspect-brk so all current IDEs will not be albe to debug node8

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

https://github.com/nodejs/node/issues/12630#issuecomment-300544480

They all had a release cycle while both protocols were available. Unfortunately they used --inspect --debug-brk to trigger inspector. That's hardcoded logic in all current versions.

[jkrems]

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

To elaborate on what @refack already said: What was stopping them was that --inspect-brk unfortunately didn't exist yet when they started to work on the integration. So they couldn't do the "right" thing.

[refack]

If I understand correctly CTC agreed to restore --inspect --debug-brk as alias to --inspect-brk, pending @ChALkeR's investigating an issue.

[Trott]

Pinging @ChALkeR: Did you gather the information you needed to gather? Are we prepared to move forward with restoring --inspect --debug-brk? Or not yet?

[joshgav]

@refack

CTC agreed to restore --inspect --debug-brk as alias to --inspect-brk, pending @ChALkeR's investigating an issue.

This is my understanding as well, and #12949 accomplishes that (and more).

@jkrems

going back to --debug (or --debug-brk w/o --inspect) anytime soon is pretty evil. The same command line option combination shouldn't trigger 2 completely different protocols across different versions of node.

I agree, and in fact I think this might be @ChALkeR's primary concern.

Based on this, #12949 should not add back --debug at all.

On the other hand, it could be helpful to our users to print a friendly message when --debug is specified in 8.x. I don't think that should be part of #12949 but would be appropriate for another PR.