Web Socket API for talking to Editor.

Web Socket API for talking to Editor.

BUG=https://code.google.com/p/dart/issues/detail?id=13901
R=devoncarew@google.com, keertip@google.com, nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28693

[Bob Nystrom, 2013-10-09 01:25:45]

OK, here's a first pass at an API. When pub serve is running, you can open a web
socket connection to it. That connection sends and receives JSON-encoded maps.
There are two commands you can send:

{"command": "uriToAsset", "uri": "<relative uri path>"}

This will take the URI path that users see in Dartium and map it back to an
"asset ID" that corresponds to that asset on the file system. This does *not*
currently take into account transformers, so only works with untransformed
assets. Since the use case is Dart debugging in Dartium, which isn't
transformed, it should be fine. Returns a map like:

{"package": "<package>", "path": "<path>"}

Where <package> is the name of the package containing the asset and <path> is
the relative path in that package (from the root) to the asset.

{"command": "assetToUri", "package": "<package>", "path": "<path>"}

As you can guess, it's the exact opposite of the above. Given an asset ID,
returns the relative URI path that asset would be served at:

{"uri": "<uri path>"}

If any of these commands have an error, an error object will be returned:

{"error": "<message>"}

Is this enough to get you started?

[devoncarew, 2013-10-09 16:34:22]

lgtm

Awesome! Looks good from the POV of the protocol.

We will want to iterate on it doing forward, either in this CL or a later one.
Specifically, we'll want the ability to provide ids to requests, so that we can
match up incoming responses to outgoing requests. This will also help
differentiate push notifications from responses. Here's a gist w/ a proposal:

https://gist.github.com/devoncarew/6897835

and a protocol that uses a similar protocol:

https://developers.google.com/chrome-developer-tools/docs/protocol/tot/page

Thoughts?

[keertip, 2013-10-09 16:43:20]

lgtm! Gives us something to work with.

[Bob Nystrom, 2013-10-09 16:53:55]

On 2013/10/09 16:34:22, devoncarew wrote:
> lgtm
> 
> Awesome! Looks good from the POV of the protocol.

\o/

> 
> We will want to iterate on it doing forward, either in this CL or a later one.

Let's do a later one.

> Specifically, we'll want the ability to provide ids to requests, so that we
can
> match up incoming responses to outgoing requests.

Works for me.

> This will also help
> differentiate push notifications from responses. Here's a gist w/ a proposal:
> 
> https://gist.github.com/devoncarew/6897835
> 
> and a protocol that uses a similar protocol:
> 
> https://developers.google.com/chrome-developer-tools/docs/protocol/tot/page
> 
> Thoughts?

SGTM!

If you want, file a bug with the changes you want and assign it to me. Nathan
and I are trying to live and die by the bug tracker until 1.0.

- bob

[nweiz, 2013-10-09 19:16:37]

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:144: /// Creates a web socket
for [request] which is should be an upgrade request.
"which is should be" -> "which should be"

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:152:
socket.add(JSON.encode({"error": '"$data" is not valid JSON.'}));
I think we should include a stack trace in the error messages. In this one in
particular, we should also include the original exception's message, in case it
provides more detailed information about the syntax error.

It would also be nice to remove some of the duplication of constructing these
error responses with a utility method. Something like "_sendError(String
message, [StackTrace stack])".

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:157:
socket.add(JSON.encode({"error": "Command must be a JSON map."}));
Include the actual value. In fact, every error message should include the actual
value.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:167: case "uriToAsset":
Using "URI" in the command names seems weird, since it's always actually going
to be a URL.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:168: var uri = new Uri(path:
command["uri"]);
Send an error response if command["uri"] is not a String.

Also, I don't think "uri" is a good name for this, since only a path is valid.
"path" would probably be better.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:169: var asset =
_uriToId(uri);
"asset" -> "id" or "assetId"

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:177: var id = new
AssetId(command["package"], command["path"]);
Send an error response if command["package"] or command["path"] is not a String.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:194: });
This should probably have cancelOnError set to true.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:216: /// Converts [id] to at
URI path that would access an asset with that ID.
"URI path" -> "root-relative URL"

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:224: "Can not serve asset from
top-level directory.");
"Can not serve asset" -> "Cannot serve assets"

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:233: return
path.join("assets", id.package, path.joinAll(parts));
All of these return values should be root-relative -- that is, they should start
with "/". Right now they're relative, but it's unclear what they're relative to.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:241: 'Can only access "web"
directory of root package.');
This is ambiguous; it could mean "of all the assets in the root package, only
those in the 'web' directory are accessible."

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:222: void
webSocketShouldReply(request, expectation, {bool encodeRequest: true}) {
Document the [encodeRequest] parameter.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:229: }), "web socket should reply");
I'd prefer a more descriptive description here. At least it should be phrased as
a description (e.g. "waiting for web socket to reply"), but it would also be
nice if it included the request and expected response as well.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/web_socket/returns_errors_on_invalid_assets_test.dart
(right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/web_socket/returns_errors_on_invalid_assets_test.dart:14:
integration("converts uris to matching asset ids", () {
Change this to match the filename.

[Bob Nystrom, 2013-10-11 20:50:10]

Thanks!

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:144: /// Creates a web socket
for [request] which is should be an upgrade request.
On 2013/10/09 19:16:37, nweiz wrote:
> "which is should be" -> "which should be"

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:152:
socket.add(JSON.encode({"error": '"$data" is not valid JSON.'}));
On 2013/10/09 19:16:37, nweiz wrote:
> I think we should include a stack trace in the error messages.

I really don't. In these cases, these are meaningful errors that pub is
intentionally reporting back to the client because they used the API wrongly. A
callstack within pub isn't relevant.

> In this one in particular, we should also include the original exception's
message, in case it
> provides more detailed information about the syntax error.

Good call. Done.

> It would also be nice to remove some of the duplication of constructing these
> error responses with a utility method. Something like "_sendError(String
> message, [StackTrace stack])".

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:157:
socket.add(JSON.encode({"error": "Command must be a JSON map."}));
On 2013/10/09 19:16:37, nweiz wrote:
> Include the actual value. In fact, every error message should include the
actual
> value.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:157:
socket.add(JSON.encode({"error": "Command must be a JSON map."}));
On 2013/10/09 19:16:37, nweiz wrote:
> Include the actual value. In fact, every error message should include the
actual
> value.

Added to some of them. In general, I don't think the error messages need to be
too user-friendly here. This is an API that will be called programmatically, not
by a human user. Once the Editor code is doing the right thing, we shouldn't see
any errors.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:167: case "uriToAsset":
On 2013/10/09 19:16:37, nweiz wrote:
> Using "URI" in the command names seems weird, since it's always actually going
> to be a URL.

I've never learned the difference between the two. :( Changed to URL.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:168: var uri = new Uri(path:
command["uri"]);
On 2013/10/09 19:16:37, nweiz wrote:
> Send an error response if command["uri"] is not a String.
> 
> Also, I don't think "uri" is a good name for this, since only a path is valid.
> "path" would probably be better.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:169: var asset =
_uriToId(uri);
On 2013/10/09 19:16:37, nweiz wrote:
> "asset" -> "id" or "assetId"

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:177: var id = new
AssetId(command["package"], command["path"]);
On 2013/10/09 19:16:37, nweiz wrote:
> Send an error response if command["package"] or command["path"] is not a
String.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:194: });
On 2013/10/09 19:16:37, nweiz wrote:
> This should probably have cancelOnError set to true.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:216: /// Converts [id] to at
URI path that would access an asset with that ID.
On 2013/10/09 19:16:37, nweiz wrote:
> "URI path" -> "root-relative URL"

This function is now in barback.dart (the pub build patch uses it too). The docs
there explain the path semantics a bit better.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:224: "Can not serve asset from
top-level directory.");
On 2013/10/09 19:16:37, nweiz wrote:
> "Can not serve asset" -> "Cannot serve assets"

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:233: return
path.join("assets", id.package, path.joinAll(parts));
On 2013/10/09 19:16:37, nweiz wrote:
> All of these return values should be root-relative -- that is, they should
start
> with "/". Right now they're relative, but it's unclear what they're relative
to.

Added TODO.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:241: 'Can only access "web"
directory of root package.');
On 2013/10/09 19:16:37, nweiz wrote:
> This is ambiguous; it could mean "of all the assets in the root package, only
> those in the 'web' directory are accessible."

Reworded.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:222: void
webSocketShouldReply(request, expectation, {bool encodeRequest: true}) {
On 2013/10/09 19:16:37, nweiz wrote:
> Document the [encodeRequest] parameter.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:229: }), "web socket should reply");
On 2013/10/09 19:16:37, nweiz wrote:
> I'd prefer a more descriptive description here. At least it should be phrased
as
> a description (e.g. "waiting for web socket to reply"), but it would also be
> nice if it included the request and expected response as well.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/web_socket/returns_errors_on_invalid_assets_test.dart
(right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/web_socket/returns_errors_on_invalid_assets_test.dart:14:
integration("converts uris to matching asset ids", () {
On 2013/10/09 19:16:37, nweiz wrote:
> Change this to match the filename.

Done.

[nweiz, 2013-10-15 23:04:48]

A couple more comments, then lgtm.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:167: case "uriToAsset":
On 2013/10/11 20:50:10, Bob Nystrom wrote:
> On 2013/10/09 19:16:37, nweiz wrote:
> > Using "URI" in the command names seems weird, since it's always actually
going
> > to be a URL.
> 
> I've never learned the difference between the two. :( Changed to URL.

My rule of thumb is that anything a browser can understand is a URL, and URIs
are a broader class that includes a bunch of things that are rarely used in
practice.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:233: return
path.join("assets", id.package, path.joinAll(parts));
On 2013/10/11 20:50:10, Bob Nystrom wrote:
> On 2013/10/09 19:16:37, nweiz wrote:
> > All of these return values should be root-relative -- that is, they should
> start
> > with "/". Right now they're relative, but it's unclear what they're relative
> to.
> 
> Added TODO.

I don't think this warrants a TODO; just add an additional "/" parameter to the
path.join call.

Also, these should be path.url.join.

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback.dart:208: // paths?
If you're claiming that it's a URL and not an ill-defined path-like thing, then
yes.

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
File
sdk/lib/_internal/pub/test/serve/web_socket/converts_asset_ids_to_urls_test.dart
(right):

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/web_socket/converts_asset_ids_to_urls_test.dart:14:
integration("converts URLs to matching asset ids", () {
Change this to match the filename.

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
File
sdk/lib/_internal/pub/test/serve/web_socket/converts_uris_to_asset_ids_test.dart
(right):

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/web_socket/converts_uris_to_asset_ids_test.dart:14:
integration("converts URLs to matching asset ids", () {
This file should be renamed as well.

[Bob Nystrom, 2013-10-16 00:01:28]

Thanks!

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:167: case "uriToAsset":
On 2013/10/15 23:04:49, nweiz wrote:
> On 2013/10/11 20:50:10, Bob Nystrom wrote:
> > On 2013/10/09 19:16:37, nweiz wrote:
> > > Using "URI" in the command names seems weird, since it's always actually
> going
> > > to be a URL.
> > 
> > I've never learned the difference between the two. :( Changed to URL.
> 
> My rule of thumb is that anything a browser can understand is a URL, and URIs
> are a broader class that includes a bunch of things that are rarely used in
> practice.

Done.

https://codereview.chromium.org/26634003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:233: return
path.join("assets", id.package, path.joinAll(parts));
On 2013/10/15 23:04:49, nweiz wrote:
> On 2013/10/11 20:50:10, Bob Nystrom wrote:
> > On 2013/10/09 19:16:37, nweiz wrote:
> > > All of these return values should be root-relative -- that is, they should
> > start
> > > with "/". Right now they're relative, but it's unclear what they're
relative
> > to.
> > 
> > Added TODO.
> 
> I don't think this warrants a TODO; just add an additional "/" parameter to
the
> path.join call.
> 
> Also, these should be path.url.join.

Done.

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback.dart:208: // paths?
On 2013/10/15 23:04:49, nweiz wrote:
> If you're claiming that it's a URL and not an ill-defined path-like thing,
then
> yes.

Done.

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
File
sdk/lib/_internal/pub/test/serve/web_socket/converts_asset_ids_to_urls_test.dart
(right):

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/web_socket/converts_asset_ids_to_urls_test.dart:14:
integration("converts URLs to matching asset ids", () {
On 2013/10/15 23:04:49, nweiz wrote:
> Change this to match the filename.

Done.

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
File
sdk/lib/_internal/pub/test/serve/web_socket/converts_uris_to_asset_ids_test.dart
(right):

https://codereview.chromium.org/26634003/diff/11001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/web_socket/converts_uris_to_asset_ids_test.dart:14:
integration("converts URLs to matching asset ids", () {
On 2013/10/15 23:04:49, nweiz wrote:
> This file should be renamed as well.

Done.

[Bob Nystrom, 2013-10-16 00:01:56]

Committed patchset #3 manually as r28693 (presubmit successful).