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

Decision to leave v8_inspector support for v6 LTS #7072

Closed
rvagg opened this issue May 31, 2016 · 25 comments
Closed

Decision to leave v8_inspector support for v6 LTS #7072

rvagg opened this issue May 31, 2016 · 25 comments
Labels
discuss Issues opened for discussions and feedbacks. inspector Issues and PRs related to the V8 inspector protocol lts Issues and PRs related to Long Term Support releases.

Comments

@rvagg
Copy link
Member

rvagg commented May 31, 2016

Attaching this to milestone 7.0.0 so we make a call prior to cutting v7 and turning v6 into LTS.

Current status: v8_inspector support has been merged in to master @ #6792, as yet it's not in v6.x but I'm anticipating that it soon will be. It adds what we are agreeing is an "experimental" feature and is hence undocumented and is caveat emptor for users. We have to make a decision prior to turning v6 into LTS in October whether we are comfortable shipping this in our binaries, experimental or not.

Current open questions, as I understand them, for us to consider in making this decision:

  • Stability: does this feature & code in anyway present a stability risk for LTS, particularly given the 30 month runway it needs to last, with Chromium moving on and presumably this protocol evolving. Is there any impact on the current debugger or any other code that we are shipping that might be cause for conern?
  • Windows support: this will probably be added shortly but if it turns out to be difficult, would be be comfortable shipping code that doesn't work across our support platforms, regardless of experimental status?
  • Reliance on Chromium on the consumer end of the protocol: are we comfortable with the level of support this enabled beyond Chromium and/or how much this creates tighter ties to the Chromium project?
  • Reliance on V8 on the internal implementation: are we comfortable the way this is tied to V8 as a V8 protocol considering we have efforts as node-chakracore and spidernode that we are attempting to foster, are these alternative VMs able to make use of this same protocol to expose similar functionality or are we comfortable with the path forward for them to do so (which may mean evolving this protocol for our own needs)?
@rvagg rvagg added this to the 7.0.0 milestone May 31, 2016
@ofrobots
Copy link
Contributor

@rvagg: Windows support has already been added. You should adjust the original post to drop this particular bullet.

@MylesBorins
Copy link
Contributor

@rvagg I was talking with @jasonLaster from Mozilla who was expressing interest in working with this api with the Mozilla dev tools. The impression that I have been getting is that various vendors are interested in building on top of this. I could imagine the communication layer as something that would be an excellent w3c standard to be used by all vendors.

Do you or anyone on the @nodejs/ctc think a Debugging WG might be a good place to discuss the majority of the above questions with various vendors? I'd be very interested in trying to find stake holders from Google, Microsoft, Mozilla, and other vendors to work with Node core on making an informed decision.

I've also gone ahead and opened nodejs/Release#109 to track all the various PR's / commits that will have to be bundled to backport this to v6.x

@rvagg
Copy link
Member Author

rvagg commented May 31, 2016

Thanks @ofrobots, that's brilliant, updated OP.

@thealphanerd a Debugging WG would be so so good to have, I've said this before but we haven't had a critical mass or an individual willing to push it forward. If you want to go ahead and start something up please coordinate with folks in the Tracing WG and anyone involved in the VM Summit as that's where a lot of folks interested in this area are currently hanging. It would be fantastic to have a group to lean on to provide recommendations, guidance and some kind of general sense of where things are heading rather than just stumbling through and reacting. @pmuellr might be available and interested in helping start something. Another alternative is to have the Tracing WG expand its remit to cover Debugging too (would probably need a rename for that), but that's up to them.

@MylesBorins
Copy link
Contributor

@rvagg thanks for the encouragement. I'll get started this week.

/cc @nodejs/tracing

@mscdex mscdex added debugger discuss Issues opened for discussions and feedbacks. lts Issues and PRs related to Long Term Support releases. labels May 31, 2016
@auchenberg
Copy link

This is highly discussion relevant for our efforts on Node tooling at Microsoft, as we already have Node and Chrome Debugging Protocol compliant debuggers available for Visual Studio and Visual Studio Code. Our WG member @joshgav has already started this conversation in the Debugging WG.

Additionally as the initiator of RemoteDebug, I already spent time with the various W3C working groups to find a home for a common (browser focused) remote debugging protocol, and the conclusion a few years ago was that it was too early to standardize this under W3C, but something among vendors should sketch out.

I've summarized the learnings in this post RemoteDebug and cross-browser DevTools. One year later., which might be relevant for this Node conversation.

@reshnm
Copy link
Contributor

reshnm commented May 31, 2016

@thealphanerd This is not directly connected to v8_inspector topic, but I did a lot of work (bugfixing and API enhancements) on the Node.js Debugger API (Debug Agent) recently for embedding in our own project. If needed, I would like to help and/or provide feedback regarding debugging topics.

@pmuellr
Copy link

pmuellr commented May 31, 2016

On the agenda for the tracing-wg call this week is the topic: "Expanding scope to Diagnostics WG", ... timely.

@benjamingr
Copy link
Member

I see a lot of benefit in shipping it as soon as possible and get usage feedback. Even if we do it behind an undocumented flag.

@joshgav
Copy link
Contributor

joshgav commented May 31, 2016

@rvagg

Reliance on Chromium on the consumer end of the protocol: are we comfortable with the level of support this enables beyond Chromium and/or how much this creates tighter ties to the Chromium project?

We'll likely gain insight by tracking the integration of support for Node+CDP in the VSCode CDP extension: microsoft/vscode-chrome-debug-core#17.


Before complete integration and official support, I think we also should consider and understand the protocol's own stability, compatibility, extensibility, and governance model. That is:

  • Stability - frequency and effect of significant changes;
  • Compatibility - support for various permutations of tools and runtime versions;
  • Extensibility - how to add (or subtract) functionality new to specific tools and runtimes;
  • Governance model - how changes to the protocol are proposed and integrated.

Both an expanded "Diagnostics WG" and the Chrome Debugging Protocol (CDP) are on the agenda for tomorrow's Tracing/Diag WG Meeting, and @ofrobots will be attending and contributing. Please join and/or provide feedback!

@ofrobots
Copy link
Contributor

/cc @pavelfeldman

@jlongster
Copy link

I work on the Firefox JavaScript debugger, and we are definitely interested in building on top of this. I already have a version of our debugger able to communicate with it. Regardless of the various details that probably need to be ironed out, we are OK with the Chrome protocol landing here. The reality is that the majority of tooling already supports some version of that protocol, so if a standardized protocol ever comes about, it will most likely be a small step-wise evolution of the Chrome protocol.

@rvagg
Copy link
Member Author

rvagg commented Jun 1, 2016

Thanks @joshgav, looking forward to having input on these issues from the WG.

@jlongster any chance we can convince you to engage with the Tracing Working Group, which is perhaps soon to become the Diagnostics Working Group? The meeting tomorrow would probably be a good place to start: nodejs/diagnostics#49

@jlongster
Copy link

@rvagg sounds great. My coworker (@jasonLaster) has already commented in there actually, but I may join as well to listen in. Should be interesting, I've been wanting this for a while.

@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol and removed debugger labels Jun 3, 2016
@Chris911
Copy link
Contributor

Any updates here?

@joshgav
Copy link
Contributor

joshgav commented Jun 15, 2016

@Chris911 see nodejs/diagnostics#59 and nodejs/diagnostics#53 where we're starting to work on goals outlined above.

@rvagg
Copy link
Member Author

rvagg commented Jul 5, 2016

Just to be absolutely clear about the purpose of the issue - it's not meant to hold up v8_inspector from landing in v6, simply that we need to ask this question just prior to v6 going LTS - do we want to continue shipping it in that version? The options would be to continue shipping it (either as experimental or official, depends on how it evolves between now and then), or to back it out of the v6 branch beforehand.

afaik we have everything resolved for this to ship in v6 now, the last outstanding thing afaik was the "experimental" note to stdout and that's in now.

@Fishrock123 Fishrock123 removed this from the 7.0.0 milestone Jul 5, 2016
@jasnell
Copy link
Member

jasnell commented Jul 5, 2016

I'm certainly +1 on shipping it in v6 with the experimental label and
warning still firmly in place.
On Jul 5, 2016 5:36 AM, "Rod Vagg" [email protected] wrote:

Just to be absolutely clear about the purpose of the issue - it's not
meant to hold up v8_inspector from landing in v6, simply that we need to
ask this question just prior to v6 going LTS - do we want to continue
shipping it in that version? The options would be to continue shipping it
(either as experimental or official, depends on how it evolves between now
and then), or to back it out of the v6 branch beforehand.

afaik we have everything resolved for this to ship in v6 now, the last
outstanding thing afaik was the "experimental" note to stdout and that's in
now.


You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#7072 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAa2ebXupjO89HX7FXXppmuDCNNkQxrjks5qSk_igaJpZM4IqKC8
.

@jasnell
Copy link
Member

jasnell commented Aug 9, 2016

Ping @nodejs/lts @nodejs/ctc ... are we close enough to make a decision on this? This far the experience with inspector has been positive and there seems to be a lot of excitement about having this. At this point I'm +1 on keeping it in v6.

@MylesBorins
Copy link
Contributor

+1 for leaving it in. I would just want verification from @ofrobots that there are no major bugs / regressions lingering

@mscdex
Copy link
Contributor

mscdex commented Aug 9, 2016

+1 from me as long as there is an experimental "label"/warning attached to it.

@evanlucas
Copy link
Contributor

+1 from me

@joshgav
Copy link
Contributor

joshgav commented Aug 9, 2016

+1 to leaving it in, but I'm concerned that the current implementation only allows it to work with V8 and V8's internal APIs. We're working hard to address that shortcoming in hindsight for runtime APIs, so we should consider it early for diagnostics APIs.

FWIW, I discussed a slight change to the architecture which would allow other engines to plug in more reasonably, see #7393.

@bnoordhuis
Copy link
Member

I don't see any reason not to keep it in so +1 from me.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 10, 2016

Definitely +1 to leaving it in v6.

@mgol
Copy link
Contributor

mgol commented Oct 25, 2016

v6 LTS is out so perhaps this should be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. inspector Issues and PRs related to the V8 inspector protocol lts Issues and PRs related to Long Term Support releases.
Projects
None yet
Development

No branches or pull requests