regression: 3rd party debuggers are incompatible with node8 nighlies

[roblourens]

In #12197, support for --inspect --debug-brk was removed, and --inspect-brk should now be used instead, but --inspect-brk is only supported after 7.6.0.
Since there is no common way to start the inspector across all Node versions that support it, this is an issue for VS Code and other debug clients which now have to determine which version of Node they are launching and select the right arguments, or detect when using one set fails, and try the other set. It's also annoying for anyone who switches node versions often and uses these arguments from the command line.

Would it be possible to retain support for --inspect --debug-brk so that there's one command which can start Node in debug mode across all versions that support the inspector protocol? If it simplifies things, it could be undocumented.

[roblourens]

@joshgav Any thoughts on this? I thought I read at one point that --debug would be an alias for --inspect in the future but I guess that's not the case anymore.

[sam-github]

Keeping uniform behaviour across all current LTS lines is quite helpful. Once all LTS nodes support --inspect-brk, users can switch over to it.

[evanlucas]

yea, I'm +1 on this

[gibfahn]

So the logic is that it's quicker to re-add --debug-brk to Node 7 than to backport --inspect-brk to Node 6 and 4 (and 4 is in maintenance so may never get it)? Makes sense to me.

[sam-github]

We could backport --inspect-brk to node 6, that would allow --debug-brk to be removed from current as soon as 4.x goes out of maintenance, rather than having to wait until 6.x goes out of maintenance.

[sam-github]

@MylesBorins @nodejs/lts what do you think of above? Should we discuss in WG meeting, or should I PR a backport?

[addaleax]

@sam-github If you want to, I think opening a PR is a good idea. There isn’t really any need to wait for a meeting if there is consensus that it should happen.

[ofrobots]

So the logic is that it's quicker to re-add --debug-brk to Node 7 than to backport --inspect-brk to Node 6 and 4 (and 4 is in maintenance so may never get it)? Makes sense to me.

--inspect is only supported since 6.3, so I don't think 4 is relevant here.

From my reading, I think the argument is slightly different than the way you put it. Without knowing the precise version upfront, I think it is hard to know whether --inspect-brk would work. It will never work for users of 6.4.0 for example because a backport would only get into 6.11.0 at the earliest.

@roblourens wouldn't you still need to distinguish Node <6.3.0 from >=6.3.0?

[sam-github]

@ofrobots, good point. I didn't realize the inspector only got introduced halfway into 6.x. So, we'll need to get the backwards-compatible --inspect --debug-brk back, and keep it until 6.x is EOL.

[gibfahn]

@ofrobots, good point. I didn't realize the inspector only got introduced halfway into 6.x. So, we'll need to get the backwards-compatible --inspect --debug-brk back, and keep it until 6.x is EOL.

@sam-github --inspect --debug-brk works fine in v6.x, --debug-brk was removed in master as a semver-major, so the question is whether we re-add it to master and Node 8.

Either way we should backport --inspect-brk to v6.x.

[sam-github]

--inspect --debug-brk works fine in v6.x

In later half of 6.x, yes, but it wasn't there initially.

And yes, I agree:

• --debug-brk should be added back to master and node 8.x
• its worth backporting --inspect-brk to 6.x

[roblourens]

@ofrobots That's right, thanks. We do still need to do version detection for which debug protocol to use, but only in the simple case when we run with Node on the user's path. But if the user provides another "runtimeExecutable", which might be a path to a shell script or 'npm' or another version of node, it's not safe to invoke that with --version, and that won't work 100% of the time anyway. So then we rely on the user to set "protocol": "inspector" to debug with --inspect. Changing the argument names would introduce another complicating wrinkle.

[refack]

Wait so in v6 --debug-brk uses debug protocol
in v7 --debug-brk uses the debug protocol
but --inspect --debug-brk uses the inspector protocol?

and node4:

C:\code\node> node4 --inspect --debug-brk
node4: bad option: --inspect

@roblourens how do you deal with this? Don't you feel fixing this issue will be a bandaid?

[refack]

So the logic is that it's quicker to re-add --debug-brk to Node 7 than to backport --inspect-brk to Node 6 and 4 (and 4 is in maintenance so may never get it)? Makes sense to me.

@gibfahn It's a wrong assumption.

1. All existing version of v4 will only work with debug protocol, and don't support --inspect --debug-brk
2. Some existing versions of v6 don't support the --inspect --debug-brk and some do.
3. All version of v7 support --inspect --debug-brk but some also support --inspect-brk
4. And the current decision is that node 8 will only support --inspect-brk

So restoring the --inspect --debug-brk will not solve all of @roblourens's problem. I'd rather go help his project then revert back to the --inspect --debug-brk combo forever.

[gibfahn]

@refack node --inspect --debug-brk will work across all versions of Node that have the inspector (if we re-add to Node 7 and Node 8), except for a couple of Node 7 versions. That doesn't solve all the problems, but it makes things simpler right? And the problem isn't just for Jetbrains, it's for Node devs who want less mental load. What's the downside to keeping this alias?

And the current decision is that node 8 will only support --inspect-brk

That decision hasn't been made, it'll probably happen in #12580.

[refack]

I made contact with JetBrains they have the same issue, but FWIW they already do version detection and select which parameter to pass. They claim version detection is robust, and had received no user complaints about the edge-case of a wrapper script as "runtimeExecutable".
I also opened microsoft/vscode-node-debug2#100 to help @roblourens with a solution on his end.

@gibfahn I believe this was discussed ad nauseum... nodejs/node-inspect#43, #12197, nodejs/CTC#94, nodejs/CTC#40, nodejs/diagnostics#67, #7266, #10276, #10187, #11770, #11207, #11431, #12352, #11421, #11441

I understand it was a "fast" deprecation because of V8 constraints, but still after considerable thought decisions were made, and I don't think this issue if enough to revert those decisions, and for a non optimal ad-hoc solution.

[MylesBorins]

Wouldn't the simplest solution be to simply keep --debug-brk as an alias for --inspect-brk in 8 and just leave it at that?

[jasnell]

The switch is not the critical part in that scenario, however. The underlying protocol changed completely. The older IDE won't be capable of supporting it without updates anyway.

[refack]

I disagree. In particular when talking about Webstorm. Someone may have bought a license for a specific version when they could afford it, but is unable to afford the cost of updates. If the only thing that is changing to break such IDEs is the removal of a switch, I think it is more reasonable to keep the switch people have come to rely upon.

@jsumners I'm talking about an update no upgrade. JetBrains and VSCode manage the communication with the nodejs debugger as an updateable plugin.
Our main concern is making sure they have the plugins ready for the release of node8. All the users will need to do is to "accept" the update...

[jsumners]

👍

[roblourens]

@refack Our node debug extension is packaged with the app and can't be updated out of band, so the only way to get a working version will be to update VS Code.

[roblourens]

By the way, what is --debug-port/--inspect-port from #12364 (comment)? I don't see where those are documented.

[prigara]

@refack

I'm talking about an update no upgrade. JetBrains and VSCode manage the communication with the nodejs debugger as an updateable plugin.

Node.js plugin is bundled with WebStorm and some other JetBrains IDEs and can not be updated without the IDE update. Normally we do not backport fixes to the previous major IDE versions. Support for --inspect --debug-brk would guarantee that the majority of our users have working Node.js debugger in WebStorm.

[refack]

@roblourens @prigara I stand corrected.
But I'll reiterate the statement that we want to make sure your products are ready for node 8. Please see dedicated discussion in #12630 (I'll quote your two last statements)

P.S. @roblourens --debug-port is an undocumented switch that sets the debug port explicitly, it was meant for IDEs and external debuggers, but I guess we failed to communicate that. (see node_debug_options.cc#L104)

[refack]

@roblourens I want to change the title of this issue to:
regression: 3rd party debuggers are incompatible with node8 nighlies
are you ok with that?

[sam-github]

@refack you as a collab can fixup issue titles, the fact that you did shows up in the conversation thread so the new text (spelling errors, etc. :-) won't be misattributed to the opener of the issue, and often issue titles stand a bit of touching up once the problem is better understood. That is, go for it.

[sam-github]

I understand your hesitation. @jasnell reworking bug report titles for clarity seems to be part of the community care we do to keep the issue tracker in good shape, not so different from adding appropriate labels, would you agree?

[jasnell]

yep, updating titles happens all the time.

[refack]

Workaround for the meanwhile:
https://gist.github.com/refack/ab07a28580672a67f8e1e0adde359aba