Provide URLs for assets from lib/ and asset/ in pub's websocket API.

Provide URLs for assets from lib/ and asset/ in pub's websocket API.

R=rnystrom@google.com
BUG=17146
Committed: https://code.google.com/p/dart/source/detail?r=34086

[Bob Nystrom, 2014-03-18 20:25:24]

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

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:132: var components =
path.url.split(id.path);
Check for zero length.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:134: components.insert(0,
id.package);
Instead of removing and then inserting, how about just assigning?

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

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:237: /// to its
location on disk. Only the "lib" and "asset" directories are
I'm still skeezed about by allowing absolute paths to packages wherever they may
lie (or be symlinked to!) on disc. How about adding a TODO to remove support for
that at some point?

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:296: /// Look up a
Don't leave me hanging!

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:300: return
_environment.servers
This is going to clash with my patch which gets rid of `servers`.

How about moving all of this into BuildEnvironment and provide a public method
that _pathToUrls can call?

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:310: if
(components.first != "packages") return [];
What about an absolute path into a "packages" directory?

/my/fs/is/so/fly/dart/stuff/myapp/packages/foo/bar.dart

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:335: return [];
Having an unreachable return is a bit ugly. How about:

Iterable<Uri> _lookUpPathInDependency(String assetPath) {
  var uri;  
  for (var package in _environment.graph.packages.values) {
    var libDir = path.join(package.dir, 'lib');
    var assetDir = path.join(package.dir, 'asset');
 
    if (path.isWithin(libDir, assetPath)) {
      uri = path.toUri(path.join('packages', package.name,
          path.relative(assetPath, from: libDir)));
      break;
    }

    if (path.isWithin(assetDir, assetPath)) {
      uri = path.toUri(path.join('assets', package.name,
          path.relative(assetPath, from: assetDir)));
      break;
    }
  }
 
  return _environment.servers.map((server) => server.url.resolveUri(uri));
}

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart
(right):

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart:66:
var path = p.join('bin', 'foo.txt');
How about a helper function to get rid of the duplication in these four tests?

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_test.dart (left):

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_test.dart:39:
schedule(() {
Thanks, I meant to remove that and forgot.

[nweiz, 2014-03-18 21:58:07]

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

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:132: var components =
path.url.split(id.path);
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> Check for zero length.

This function is left over from a different approach than I ended up using.
Removed.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/server.dart:134: components.insert(0,
id.package);
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> Instead of removing and then inserting, how about just assigning?

Ditto.

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

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:237: /// to its
location on disk. Only the "lib" and "asset" directories are
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> I'm still skeezed about by allowing absolute paths to packages wherever they
may
> lie (or be symlinked to!) on disc. How about adding a TODO to remove support
for
> that at some point?

I's prefer to wait and see how the whole editor/analyzer/pub situation with
dependency assets shakes out in a non-symlink world. I think it's possible that
the editor's display of a dependency file will come from the pub cache location,
in which case this will be very useful.

It also makes the experience editing multiple sibling packages with path
dependencies better.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:296: /// Look up a
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> Don't leave me hanging!

Don

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:300: return
_environment.servers
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> This is going to clash with my patch which gets rid of `servers`.
> 
> How about moving all of this into BuildEnvironment and provide a public method
> that _pathToUrls can call?

Done.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:310: if
(components.first != "packages") return [];
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> What about an absolute path into a "packages" directory?
> 
> /my/fs/is/so/fly/dart/stuff/myapp/packages/foo/bar.dart

Done.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:335: return [];
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> Having an unreachable return is a bit ugly. How about:
> 
> Iterable<Uri> _lookUpPathInDependency(String assetPath) {
>   var uri;  
>   for (var package in _environment.graph.packages.values) {
>     var libDir = path.join(package.dir, 'lib');
>     var assetDir = path.join(package.dir, 'asset');
>  
>     if (path.isWithin(libDir, assetPath)) {
>       uri = path.toUri(path.join('packages', package.name,
>           path.relative(assetPath, from: libDir)));
>       break;
>     }
> 
>     if (path.isWithin(assetDir, assetPath)) {
>       uri = path.toUri(path.join('assets', package.name,
>           path.relative(assetPath, from: assetDir)));
>       break;
>     }
>   }
>  
>   return _environment.servers.map((server) => server.url.resolveUri(uri));
> }

Huh? This is totally reachable -- if none of the environment's packages contain
[assetPath], we'll reach this.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart
(right):

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/web_socket/path_to_urls_errors_test.dart:66:
var path = p.join('bin', 'foo.txt');
On 2014/03/18 20:25:24, Bob Nystrom wrote:
> How about a helper function to get rid of the duplication in these four tests?

Done.

[Bob Nystrom, 2014-03-18 23:59:31]

lgtm

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

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:237: /// to its
location on disk. Only the "lib" and "asset" directories are
On 2014/03/18 21:58:08, nweiz wrote:
> On 2014/03/18 20:25:24, Bob Nystrom wrote:
> > I'm still skeezed about by allowing absolute paths to packages wherever they
> may
> > lie (or be symlinked to!) on disc. How about adding a TODO to remove support
> for
> > that at some point?
> 
> I's prefer to wait and see how the whole editor/analyzer/pub situation with
> dependency assets shakes out in a non-symlink world. I think it's possible
that
> the editor's display of a dependency file will come from the pub cache
location,
> in which case this will be very useful.
> 
> It also makes the experience editing multiple sibling packages with path
> dependencies better.

SGTM.

https://codereview.chromium.org/203413004/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/web_socket_api.dart:335: return [];
On 2014/03/18 21:58:08, nweiz wrote:
> On 2014/03/18 20:25:24, Bob Nystrom wrote:
> > Having an unreachable return is a bit ugly. How about:
> > 
> > Iterable<Uri> _lookUpPathInDependency(String assetPath) {
> >   var uri;  
> >   for (var package in _environment.graph.packages.values) {
> >     var libDir = path.join(package.dir, 'lib');
> >     var assetDir = path.join(package.dir, 'asset');
> >  
> >     if (path.isWithin(libDir, assetPath)) {
> >       uri = path.toUri(path.join('packages', package.name,
> >           path.relative(assetPath, from: libDir)));
> >       break;
> >     }
> > 
> >     if (path.isWithin(assetDir, assetPath)) {
> >       uri = path.toUri(path.join('assets', package.name,
> >           path.relative(assetPath, from: assetDir)));
> >       break;
> >     }
> >   }
> >  
> >   return _environment.servers.map((server) => server.url.resolveUri(uri));
> > }
> 
> Huh? This is totally reachable -- if none of the environment's packages
contain
> [assetPath], we'll reach this.

Argh, right. Ignore me.

[nweiz, 2014-03-19 00:20:26]

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