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

Use "Service" instead of "_Service" for VM service protocol >= 3.22 #1844

Merged
merged 2 commits into from Jul 10, 2019

Conversation

DanTup
Copy link
Member

@DanTup DanTup commented Jul 9, 2019

@devoncarew would you mind scanning these changes? (Select Hide whitespace changes in the diff to avoid seeing loads of changes due to indent change!).

Fixes #1843.
Also see dart-archive/vm_service_drivers#251.

@DanTup DanTup added is non-functional in debugger Relates to the debug adapter or process of launching a debug session labels Jul 9, 2019
@DanTup DanTup added this to the v3.3.0 milestone Jul 9, 2019
this.observatory.on("Extension", (event: VMEvent) => this.handleExtensionEvent(event));
this.observatory.on("Debug", (event: VMEvent) => this.handleDebugEvent(event));

const serviceStreamName = this.capabilities.serviceStreamIsPublic ? "Service" : "_Service";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - good to be able to switch on the service protocol version in general.

I think you'd also be good just subscribing to both streams and ignoring error results.

Perhaps add a todo: to remove the switch at some point?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd also be good just subscribing to both streams and ignoring error results.

That was my original plan, but making sure that if they both failed we had something in the log to debug (without always writing logs for the one that failed) was messy, so I figured I may as well do it properly (it seems likely we'll need this if changes for breakpoints across multiple debuggers happen anyway).

Perhaps add a todo: to remove the switch at some point?

Rather than TODOs all around the place I've started periodically reviewing the Capabilities classes (of which there are now quite a few now) and chopping properties for old things out and fixing the related code appropriately. Unless they're adding complexity, I've been trying to keep them around a while though - this one seems pretty trivial (fun fact: most days Dart Code still has hundreds of users using SDK v1.24 🙃!)

@DanTup
Copy link
Member Author

DanTup commented Jul 10, 2019

The red builds are all from Flutter Web test failures.. they've always been a bit flaky on the CIs - though seems worse lately. They're all passing locally for this branch so I'm confident nothing is broken by it. I'll work on making them better when I do work to make them run on the unforked version.

@DanTup DanTup merged commit f011fe8 into master Jul 10, 2019
@DanTup DanTup deleted the vm-service-protocol-changes branch July 10, 2019 08:02
@DanTup DanTup modified the milestones: v3.3.0, v3.2.1 Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in debugger Relates to the debug adapter or process of launching a debug session is non-functional
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update code for VM Service 3.22 changes
2 participants