Tweaks to browser-launching service #563

DanTup · 2019-04-22T13:16:23Z

I made some tweaks while wiring this up in VS Code.

id field now supports any type (as per JSON-RPC) - specifically, VS Code was using strings for all the other services (not for any good reason, but it seemed better to support this than fail)
The success flag is a simple boolean rather than using the VM-Service Success() class (since this didn't seem to match the other stdio services we have). Success() is still used for the response of the VM-Service handler though.
Handle all valid URIs (for ex. http:// without the /ws- what was accepted, but failed)
Check the method field before processing the incoming message (and provide a useful message if the method is not recognised)

natebosch · 2019-04-22T17:00:44Z

packages/devtools_server/lib/src/server.dart

@@ -125,39 +125,57 @@ void serveDevTools({
  //   }
  // }
  _stdinCommandStream.listen((Map<String, dynamic> json) async {
-    final int id = json['id'];
+    final dynamic id = json['id'];


What types might we get here?

VS Code has always used strings. When I changed this, I thought I checked the spec and it said "any value", however that was actually Wikipedia! The spec says:

An identifier established by the Client that MUST contain a String, Number, or NULL value if included.

I don't think we follow JSON-RPC particularly strictly, but I do think we should support both int+string here (I'm using shared code used for Flutter/analysis server/etc. which used strings, and it was just crashing on this line).

I'm hoping one day we can support int | String!

(I've added a comment about the types)

The comment helps a lot. Thanks!

natebosch · 2019-04-22T17:01:32Z

packages/devtools_server/lib/src/server.dart

  });
 }

+Future _handleVmRegister(dynamic id, Map<String, dynamic> params,


[nit] How about Future<void> instead of Future<dynamic> return?

Done! I think I used Extract Method and didn't spot it - thanks!

* Move server code to devtools_server package

jacob314 · 2019-04-22T20:46:31Z

packages/devtools_server/lib/src/server.dart

+  // Lots of things are considered valid URIs (including empty strings
+  // and single letters) since they can be relative, so we need to do some
+  // extra checks.
+  if (uri != null &&


while you are here we should really be failing if the uri is not valid instead of just quietly pretending everything succeeded.

Good spot - this code wasn't sending any response back (so if it happened, VS Code would just sit waiting). It also would throw on the Uri.parse instead of giving a well-formed error, so I've changed that too.

jacob314 · 2019-04-22T20:48:49Z

packages/devtools_server/lib/src/server.dart

@@ -233,3 +259,17 @@ void printOutput(
 }) {
  print(machineMode ? jsonEncode(json) : message);
 }
+
+/// Map the URI (which may already be Observatory web app) to a WebSocket URI


Add a line break after :"from the VM service."
The title of an comment should be on a separate line from following sentences.

jacob314 · 2019-04-22T20:52:24Z

packages/devtools_server/lib/src/server.dart

+/// Map the URI (which may already be Observatory web app) to a WebSocket URI
+/// for the VM service. If the URI is already a VM Service WebSocket URI it
+/// will not be modified.
+Uri getVmServiceUriFromObservatoryUri(Uri uri) {


Instead of duplicating this method from
package:devtools we should add this method to
package:vm_service_lib
so we can use it everywhere.

Makes sense - I've added a TODO, and will open a PR in vm_service_lib today.

PR is at dart-archive/vm_service_drivers#232

This is to make it easier to reuse in devtools+devtools_server. See flutter/devtools#563 (comment).

jacob314

* Add helper to convert Observatory URI to WS URI This is to make it easier to reuse in devtools+devtools_server. See flutter/devtools#563 (comment). * Simplify test * Add changelog + rev version

kenzieschmoll · 2019-04-23T16:54:53Z

packages/devtools_server/lib/src/server.dart

+/// for the VM service.
+///
+/// If the URI is already a VM Service WebSocket URI it will not be modified.
+Uri getVmServiceUriFromObservatoryUri(Uri uri) {


Does the naming of this method still make sense? Seems odd to me that we have observatory in the name.

Ah, that is a good point.

I wasn't sure what to call it. The banner says "Observatory listening on" and then gives a HTTP URL. The service URI we want is a websocket protocol and has /ws on the end. What should we call each of them?

getVmServiceUriFromObservatoryUri() ==> convertToWebsocketUrl()? Or,

String convertToWebsocketUrl({@required String serviceProtocolUrl}) { ... }

That works for me - though I think making it named is a bit overkill, as it's never likely to be a literal string, so we'll always end up with convertToWebSocketUrl(serviceProtocolUrl: serviceProtocolUrl).

Also, since the types are Uris, we should probably use Uri?

Uri convertToWebsocketUri(Uri serviceProtocolUri) { ... }

I still think "serviceProtocolUri" to mean the non-websocket part is weird, but I don't the the specific name is that important as long as it conveys roughly what it does - the doc comments can always be more detailed if required.

@devoncarew I haven't made any code use this yet, so changing it and re-publishing is fine by me. Since I can't merge publish, feel free to just change it if it's easier than waiting for me.

natebosch · 2019-04-23T19:54:28Z

packages/devtools_server/lib/src/server.dart

+/// for the VM service.
+///
+/// If the URI is already a VM Service WebSocket URI it will not be modified.
+Uri getVmServiceUriFromObservatoryUri(Uri uri) {


[nit] https://www.dartlang.org/guides/language/effective-dart/design#avoid-starting-a-method-name-with-get

I should re-read that doc periodically till it sinks in =)

(I'm surprised we don't have a lint for this one - at least, I can't see one)

@pq We should have a lint and quick fix that catches zero argument methods where the name starts with "get" and quick fixes them to be actual getters.

DanTup · 2019-04-24T14:58:20Z

I've pushed 347d1c8 that removes the method discussed above, in favour of the shared one in vm_service_libs.

It won't build until dart-archive/vm_service_drivers#233 lands and is published to pub, but if the code is LGTM'd in the meantime, I can just rebuild when that lands and if it goes green, land it.

DanTup · 2019-04-24T15:48:20Z

Forcing travis now the new build is out. Can someone check with the latest change (347d1c8) this is good to land?

DanTup · 2019-04-25T09:55:14Z

I've reviewed the failures here, and believe they're all the same timing-related flakes we're seeing on master right now.

@jacob314 still happy for me to land this? I've removed the helper function that was duplicated (and it's ben renamed in the new place) since your LGTM, but otherwise I think it's the same.

kenzieschmoll

approved with one nit

kenzieschmoll · 2019-04-25T15:14:30Z

packages/devtools_server/lib/src/server.dart

+        break;
+      default:
+        printOutput(
+          'Unknown command ${json['method']}',


nit: should this say "Unknown method" to match the 'error' param below it?

JSON-RPC allows for this, and other Flutter services handle strings.

Success() ('type': 'success') seems to be a VM-Service convention but other stdio services like Flutter just have a bool.

Without this, an error occurs if you send a HTTP URI since it tries to connect to it as a websocket. This also adds /ws on the end if it's not there. This makes it valid to send the URL that's printed by the VM directly.

We will remove the `method` one once VS Code has been updated for a while.

Removes both copies of `getVmServiceUriFromObservatoryUri` and calls the shared `convertToWebSocketUrl` method.

DanTup · 2019-04-25T15:32:47Z

Fixed the error and rebased to get the flaky test fixes. Will land when (if) it goes green.

DanTup mentioned this pull request Apr 22, 2019

P0: VS Code: Delegate launching browser to DevTools to always get Chrome Dart-Code/Dart-Code#1505

Closed

natebosch reviewed Apr 22, 2019

View reviewed changes

DanTup referenced this pull request Apr 22, 2019

Move server code to devtools_server package (#539)

1efd6e0

* Move server code to devtools_server package

jacob314 reviewed Apr 22, 2019

View reviewed changes

DanTup added a commit to DanTup/vm_service_drivers that referenced this pull request Apr 23, 2019

Add helper to convert Observatory URI to WS URI

5ada73a

This is to make it easier to reuse in devtools+devtools_server. See flutter/devtools#563 (comment).

DanTup mentioned this pull request Apr 23, 2019

Add helper to convert Observatory URI to WS URI dart-archive/vm_service_drivers#232

Merged

jacob314 approved these changes Apr 23, 2019

View reviewed changes

kenzieschmoll reviewed Apr 23, 2019

View reviewed changes

natebosch reviewed Apr 23, 2019

View reviewed changes

DanTup force-pushed the launch-browser-tweaks branch from 347d1c8 to 1890569 Compare April 24, 2019 16:06

jacob314 approved these changes Apr 25, 2019

View reviewed changes

kenzieschmoll approved these changes Apr 25, 2019

View reviewed changes

DanTup added 12 commits April 25, 2019 16:32

Allow ID to be any type

609fe73

JSON-RPC allows for this, and other Flutter services handle strings.

Change success flag for non-VM-service to simple bool

b86c187

Success() ('type': 'success') seems to be a VM-Service convention but other stdio services like Flutter just have a bool.

Ensure we handle all types of valid VM service URIs

e8c4b9a

Without this, an error occurs if you send a HTTP URI since it tries to connect to it as a websocket. This also adds /ws on the end if it's not there. This makes it valid to send the URL that's printed by the VM directly.

Use the method field to decide how to handle the request

f1a1caa

Send both event and method for compatibility

a75b75e

We will remove the `method` one once VS Code has been updated for a while.

Comment valid types for IDs

bb80e3e

Fix return type

a794d89

Ensure we always respond to vm.register even if URLs are invalid

8addfa3

Fix comment

3a6a9c5

Add a TODO

cecfec5

Move to vm_service_lib-provided helper

3a19128

Removes both copies of `getVmServiceUriFromObservatoryUri` and calls the shared `convertToWebSocketUrl` method.

Fix error message

faba3b9

DanTup force-pushed the launch-browser-tweaks branch from 0e58d4d to faba3b9 Compare April 25, 2019 15:32

DanTup merged commit a42f331 into flutter:master Apr 25, 2019

DanTup deleted the launch-browser-tweaks branch April 25, 2019 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweaks to browser-launching service #563

Tweaks to browser-launching service #563

DanTup commented Apr 22, 2019

natebosch Apr 22, 2019

DanTup Apr 22, 2019

DanTup Apr 22, 2019

natebosch Apr 22, 2019

natebosch Apr 22, 2019

DanTup Apr 22, 2019

jacob314 Apr 22, 2019

DanTup Apr 23, 2019

jacob314 Apr 22, 2019

DanTup Apr 23, 2019

jacob314 Apr 22, 2019

DanTup Apr 23, 2019

DanTup Apr 23, 2019

jacob314 left a comment

kenzieschmoll Apr 23, 2019

devoncarew Apr 23, 2019

DanTup Apr 23, 2019

devoncarew Apr 23, 2019

DanTup Apr 23, 2019

natebosch Apr 23, 2019

DanTup Apr 23, 2019

jacob314 Apr 25, 2019

DanTup commented Apr 24, 2019

DanTup commented Apr 24, 2019

DanTup commented Apr 25, 2019

kenzieschmoll left a comment

kenzieschmoll Apr 25, 2019

DanTup commented Apr 25, 2019

Tweaks to browser-launching service #563

Tweaks to browser-launching service #563

Conversation

DanTup commented Apr 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanTup commented Apr 24, 2019

DanTup commented Apr 24, 2019

DanTup commented Apr 25, 2019

kenzieschmoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanTup commented Apr 25, 2019