First stab at a dev server in pub using barback.

First stab at a dev server in pub using barback.

R=nweiz@google.com

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

[Bob Nystrom, 2013-07-25 00:23:33]

Very bare bones, but it's a start.

It handles locating assets and libraries in the applications web, lib, and asset
directories, as well as dependency libraries and assets in lib and asset.

Doesn't do any transformations, and doesn't notice when source assets have been
modified yet.

[nweiz, 2013-07-25 22:58:58]

This needs tests for 404s in packages and assets directories, especially with
odd paths like "/packages".

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.dart
File pkg/barback/lib/src/barback.dart (right):

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.d...
pkg/barback/lib/src/barback.dart:15: /// A general purpose asynchronous build
dependency graph manager.
"General purpose" should be hyphenated.

I want a little more description than this about barback's purpose before you
start diving into how to use it. "Build dependency graph manager" can mean a lot
of things. Talk about how it takes assets (including Dart files) from multiple
packages, runs package-specific transformations on them, and tracks when the
transformations need to be re-run.

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.d...
pkg/barback/lib/src/barback.dart:18: /// and raw source [Asset]s. Then you add
input files to the system by calling
"input files to the system" is confusing. [updateSources] doesn't actually
provide it with files; it does a sort of combination of telling it which files
exist and when they get updated.

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.d...
pkg/barback/lib/src/barback.dart:20: /// transformers to those inputs and start
running them asynchronously. If the
"the transformer" -> "a transformer"

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:42: }
Style nit: put this in a getter to avoid the awkward "var port;" declaration.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:46:
barback.results.listen((result) {
Add a TODO to report errors and successful build completions via
growl/notify-send if available.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:61:
barback.updateSources(provider.listAssets(package));
Add a TODO to re-update these sources when they change on disk.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:64:
HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, port).then((server) {
Doesn't "localhost" work these days?

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:66: "on
http://localhost:${server.port}...");
The "..." here may make copy/pasting this URL difficult.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:68: server.listen((HttpRequest
request) {
Don't type annotate [request].

We should check that the request is GET or HEAD.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:73:
request.response.addStream(asset.read()).then((_) {
Add a TODO to set the Content-Type based on the extension.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:74: request.response.close();
I thought [addStream] automatically closed the consumer when the stream passed
in was closed. I may be wrong, though.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:75: });
We should handle errors that come off this future.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:77: if (error is!
AssetNotFoundException) throw error;
This will top-level the error. That's not what we want. We should pipe this to
pub's error handling.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:80:
request.response.reasonPhrase = error.toString();
The reason phrase is expected to be very brief, and almost always the standard
phrases for each code (e.g. "Not Found" for 404). Reason phrases will also not
be displayed in browsers. Custom data like the error string should go in the
response body.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:82: return;
Remove.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:89: return new
Completer().future;
This seems, as you say, hacky. It feels like we should return a future that will
complete to any programmatic errors that occur in barback or in our server.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:95: var relative =
uri.path.substring(1);
Rather than explicitly using substring, it seems cleaner and more reliable to
use pathos. [split] will always put the absolute (or in this case root-relative)
prefix as the first element, and you can just remove that.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:96: if (relative == "")
relative = "index.html";
Supporting "index.html" only for the root seems wrong. If http://localhost:8080/
routes to web/index.html, then http://localhost:8080/docs should route to
web/docs/index.html.

I don't know that this is the correct place to handle this, since in order to
properly determine whether index.html should be added, we need to look at the
filesystem. Maybe [PubPackageProvider.loadAsset] should handle it?

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:98: var parts =
path.split(relative);
This will break on windows. You need to make a URL-specific Builder.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:101: // named [name]. If so,
maps the path after that to be within [dir].
This isn't strictly accurate; it treats the directory immediately after [name]
as the package, then puts the path after *that* in the AssetId's [path]
component under [dir].

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:109: return new
AssetId(package, assetPath);
Right now this will crash on paths like "packages" or "packages/foo". It should
do something that leads to a 404 in those cases.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:126: class PubPackageProvider
implements PackageProvider {
This is going to be used elsewhere pretty soon. Might as well give it its own
library now.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:149: });
Rather than manually expanding it, just use
`entrypoint.loadLockFile().packages.map`.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:160: List<AssetId>
listAssets(String package, {String within}) {
Right now [listAssets] is used nowhere in barback. The semantics of it are also
unclear; is it supposed to return everything in a package? Just things it
considers to be "assets"? What should it consider to be assets?

I think we should remove this from the PackageProvider interface for now until
we have need for it in barback.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:166: if (!dir.existsSync())
return;
Use [dirExists].

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:167: var entries =
dir.listSync(recursive: true);
Use [listDir] rather than [Directory.listSync].

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:189: }
This shouldn't list the contents of packages directories.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:195: var file =
path.normalize(path.join(_packageDirs[id.package], id.path));
What's the value of normalizing the path here?

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:56: return new Future(() {
newFuture

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

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/404_on_missing_file_test.dart:1: // Copyright
(c) 2012, the Dart project authors.  Please see the AUTHORS d.file
2013. Same for other new test files.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/404_on_missing_file_test.dart:3: // BSD-style
license that can be found in the LICENSE d.file.
"LICENSE file."

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

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/serve_from_app_test.dart:36:
integration("'packages' URLs look in the app's lib directory", () {
I thought we were on a strict one-test-per-file diet in pub.

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

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:28: schedule(() =>
_pubServer.nextLine().then((line) {
[ScheduledProcess.nextLine] is automatically scheduled, so there's no need for
the extra [schedule] call here.

[Bob Nystrom, 2013-07-26 20:20:11]

Thanks!

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.dart
File pkg/barback/lib/src/barback.dart (right):

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.d...
pkg/barback/lib/src/barback.dart:15: /// A general purpose asynchronous build
dependency graph manager.
On 2013/07/25 22:58:58, nweiz wrote:
> "General purpose" should be hyphenated.
> 
> I want a little more description than this about barback's purpose before you
> start diving into how to use it. "Build dependency graph manager" can mean a
lot
> of things. Talk about how it takes assets (including Dart files) from multiple
> packages, runs package-specific transformations on them, and tracks when the
> transformations need to be re-run.

Done.

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.d...
pkg/barback/lib/src/barback.dart:18: /// and raw source [Asset]s. Then you add
input files to the system by calling
On 2013/07/25 22:58:58, nweiz wrote:
> "input files to the system" is confusing. [updateSources] doesn't actually
> provide it with files; it does a sort of combination of telling it which files
> exist and when they get updated.

Done.

https://codereview.chromium.org/20204003/diff/1/pkg/barback/lib/src/barback.d...
pkg/barback/lib/src/barback.dart:20: /// transformers to those inputs and start
running them asynchronously. If the
On 2013/07/25 22:58:58, nweiz wrote:
> "the transformer" -> "a transformer"

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:42: }
On 2013/07/25 22:58:58, nweiz wrote:
> Style nit: put this in a getter to avoid the awkward "var port;" declaration.

It is a bit awkward, but I'm not crazy about a getter that can call exit() based
on user-provided data. I like that this is being checked explicitly at a
specific point in the command's run code.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:46:
barback.results.listen((result) {
On 2013/07/25 22:58:58, nweiz wrote:
> Add a TODO to report errors and successful build completions via
> growl/notify-send if available.

Do we actually intend to do that? That seems painfully chatty. Builds will run
in theory every time you Ctrl-S some source file.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:61:
barback.updateSources(provider.listAssets(package));
On 2013/07/25 22:58:58, nweiz wrote:
> Add a TODO to re-update these sources when they change on disk.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:64:
HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, port).then((server) {
On 2013/07/25 22:58:58, nweiz wrote:
> Doesn't "localhost" work these days?

Not sure. I figured an enum is better than a magic string. Do you have a
preference?

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:66: "on
http://localhost:${server.port}...");
On 2013/07/25 22:58:58, nweiz wrote:
> The "..." here may make copy/pasting this URL difficult.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:68: server.listen((HttpRequest
request) {
On 2013/07/25 22:58:58, nweiz wrote:
> Don't type annotate [request].

Done.

> We should check that the request is GET or HEAD.

How should it respond to other methods? 404?

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:73:
request.response.addStream(asset.read()).then((_) {
On 2013/07/25 22:58:58, nweiz wrote:
> Add a TODO to set the Content-Type based on the extension.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:74: request.response.close();
On 2013/07/25 22:58:58, nweiz wrote:
> I thought [addStream] automatically closed the consumer when the stream passed
> in was closed. I may be wrong, though.

Nope:

"When the IOSink is bound to a stream (through addStream) any call to the IOSink
will throw a StateError. When the addStream compeltes, the IOSink will again be
open for all calls."

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:75: });
On 2013/07/25 22:58:58, nweiz wrote:
> We should handle errors that come off this future.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:77: if (error is!
AssetNotFoundException) throw error;
On 2013/07/25 22:58:58, nweiz wrote:
> This will top-level the error. That's not what we want. We should pipe this to
> pub's error handling.

Done. I log it and respond with a 500. Seems about right for a dev server, I
think?

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:80:
request.response.reasonPhrase = error.toString();
On 2013/07/25 22:58:58, nweiz wrote:
> The reason phrase is expected to be very brief, and almost always the standard
> phrases for each code (e.g. "Not Found" for 404). Reason phrases will also not
> be displayed in browsers. Custom data like the error string should go in the
> response body.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:82: return;
On 2013/07/25 22:58:58, nweiz wrote:
> Remove.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:89: return new
Completer().future;
On 2013/07/25 22:58:58, nweiz wrote:
> This seems, as you say, hacky. It feels like we should return a future that
will
> complete to any programmatic errors that occur in barback or in our server.

In the above code, I'm catching all errors that come from getAssetById() (which
effectively means almost all errors) and returning a 500 and continuing. We're
also logging the errors from barback's error stream. I don't think that leaves
much else to report.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:95: var relative =
uri.path.substring(1);
On 2013/07/25 22:58:58, nweiz wrote:
> Rather than explicitly using substring, it seems cleaner and more reliable to
> use pathos. [split] will always put the absolute (or in this case
root-relative)
> prefix as the first element, and you can just remove that.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:96: if (relative == "")
relative = "index.html";
On 2013/07/25 22:58:58, nweiz wrote:
> Supporting "index.html" only for the root seems wrong. If
http://localhost:8080/
> routes to web/index.html, then http://localhost:8080/docs should route to
> web/docs/index.html.
> 
> I don't know that this is the correct place to handle this, since in order to
> properly determine whether index.html should be added, we need to look at the
> filesystem.

That the nice thing about "/". For that one case, you don't need to hit the
filesystem because there can't be an unnamed file. For "localhost/docs" that
could be a file named "docs" or "docs/index.html". There's no easy way to tell.

> Maybe [PubPackageProvider.loadAsset] should handle it?

That doesn't feel right to me either. From Barback's perspective,
"docs/index.html" and "docs" should be the same asset. We wouldn't want to run a
transformer separately on each of those.

I added a TODO here. I think the "/" case is simple and useful enough to be
worth keeping. I'd like to do something similar for other URLs too, but that's
trickier.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:98: var parts =
path.split(relative);
On 2013/07/25 22:58:58, nweiz wrote:
> This will break on windows. You need to make a URL-specific Builder.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:101: // named [name]. If so,
maps the path after that to be within [dir].
On 2013/07/25 22:58:58, nweiz wrote:
> This isn't strictly accurate; it treats the directory immediately after [name]
> as the package, then puts the path after *that* in the AssetId's [path]
> component under [dir].

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:101: // named [name]. If so,
maps the path after that to be within [dir].
On 2013/07/25 22:58:58, nweiz wrote:
> This isn't strictly accurate; it treats the directory immediately after [name]
> as the package, then puts the path after *that* in the AssetId's [path]
> component under [dir].

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:109: return new
AssetId(package, assetPath);
On 2013/07/25 22:58:58, nweiz wrote:
> Right now this will crash on paths like "packages" or "packages/foo". It
should
> do something that leads to a 404 in those cases.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:126: class PubPackageProvider
implements PackageProvider {
On 2013/07/25 22:58:58, nweiz wrote:
> This is going to be used elsewhere pretty soon. Might as well give it its own
> library now.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:149: });
On 2013/07/25 22:58:58, nweiz wrote:
> Rather than manually expanding it, just use
> `entrypoint.loadLockFile().packages.map`.

packages is a Map, not an Iterable, so I tried this but it ends up being more
verbose to use .keys.map().

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:160: List<AssetId>
listAssets(String package, {String within}) {
On 2013/07/25 22:58:58, nweiz wrote:
> Right now [listAssets] is used nowhere in barback. The semantics of it are
also
> unclear; is it supposed to return everything in a package? Just things it
> considers to be "assets"? What should it consider to be assets?

It was being used forever ago when I was prototyping plug-in loading.

> I think we should remove this from the PackageProvider interface for now until
> we have need for it in barback.

Good call. Done.

I left the concrete method here because it's the most convenient place for it
since it needs access to the map of package directories.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:167: var entries =
dir.listSync(recursive: true);
On 2013/07/25 22:58:58, nweiz wrote:
> Use [listDir] rather than [Directory.listSync].

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:189: }
On 2013/07/25 22:58:58, nweiz wrote:
> This shouldn't list the contents of packages directories.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:195: var file =
path.normalize(path.join(_packageDirs[id.package], id.path));
On 2013/07/25 22:58:58, nweiz wrote:
> What's the value of normalizing the path here?

Not much. Removed.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/source/path.dart (right):

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/source/path.dart:56: return new Future(() {
On 2013/07/25 22:58:58, nweiz wrote:
> newFuture

Done.

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

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/404_on_missing_file_test.dart:1: // Copyright
(c) 2012, the Dart project authors.  Please see the AUTHORS d.file
On 2013/07/25 22:58:58, nweiz wrote:
> 2013. Same for other new test files.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/404_on_missing_file_test.dart:3: // BSD-style
license that can be found in the LICENSE d.file.
On 2013/07/25 22:58:58, nweiz wrote:
> "LICENSE file."

Ha, done. :)

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

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/serve_from_app_test.dart:36:
integration("'packages' URLs look in the app's lib directory", () {
On 2013/07/25 22:58:58, nweiz wrote:
> I thought we were on a strict one-test-per-file diet in pub.

Done.

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

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:28: schedule(() =>
_pubServer.nextLine().then((line) {
On 2013/07/25 22:58:58, nweiz wrote:
> [ScheduledProcess.nextLine] is automatically scheduled, so there's no need for
> the extra [schedule] call here.

Done.

[nweiz, 2013-07-29 20:23:14]

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:46:
barback.results.listen((result) {
On 2013/07/26 20:20:11, Bob Nystrom wrote:
> On 2013/07/25 22:58:58, nweiz wrote:
> > Add a TODO to report errors and successful build completions via
> > growl/notify-send if available.
> 
> Do we actually intend to do that? That seems painfully chatty. Builds will run
> in theory every time you Ctrl-S some source file.

In my experience, people like it. We can be smart about not printing it if the
build completes fast enough, and barback will already roll together multiple
builds into one, so if files get saved quickly in succession it won't show
something for all of them.

If it's worth printing info on the command line, it's probably worth putting it
somewhere the user is likely to see as well.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:64:
HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, port).then((server) {
On 2013/07/26 20:20:11, Bob Nystrom wrote:
> On 2013/07/25 22:58:58, nweiz wrote:
> > Doesn't "localhost" work these days?
> 
> Not sure. I figured an enum is better than a magic string. Do you have a
> preference?

We're using "localhost" below, and "127.0.0.1" elsewhere. The constant is
verbose and ultimately less clear than just "localhost". I'd rather use the name
that's more familiar.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:75: });
On 2013/07/26 20:20:11, Bob Nystrom wrote:
> On 2013/07/25 22:58:58, nweiz wrote:
> > We should handle errors that come off this future.
> 
> Done.

Unlike errors coming off [getAssetById], errors coming from [addStream] or
[asset.read] probably should result in a 500 error, since they're likely to be
transient things like filesystem hiccups.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:77: if (error is!
AssetNotFoundException) throw error;
On 2013/07/26 20:20:11, Bob Nystrom wrote:
> On 2013/07/25 22:58:58, nweiz wrote:
> > This will top-level the error. That's not what we want. We should pipe this
to
> > pub's error handling.
> 
> Done. I log it and respond with a 500. Seems about right for a dev server, I
> think?

The only error getAssetById is expected to raise is AssetNotFoundException.
Anything else is caused by some unexpected condition, which may involve barback
being in an inconsistent state -- one that may not be localized to the asset in
question. That should bring down the server so that it doesn't start doing bad
things.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:89: return new
Completer().future;
On 2013/07/26 20:20:11, Bob Nystrom wrote:
> On 2013/07/25 22:58:58, nweiz wrote:
> > This seems, as you say, hacky. It feels like we should return a future that
> will
> > complete to any programmatic errors that occur in barback or in our server.
> 
> In the above code, I'm catching all errors that come from getAssetById()
(which
> effectively means almost all errors) and returning a 500 and continuing. We're
> also logging the errors from barback's error stream. I don't think that leaves
> much else to report.

As with [getAssetById], programmatic errors in barback likely indicate an
inconsistent state that should cause us to bail.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:96: if (relative == "")
relative = "index.html";
On 2013/07/26 20:20:11, Bob Nystrom wrote:
> On 2013/07/25 22:58:58, nweiz wrote:
> > Supporting "index.html" only for the root seems wrong. If
> http://localhost:8080/
> > routes to web/index.html, then http://localhost:8080/docs should route to
> > web/docs/index.html.
> > 
> > I don't know that this is the correct place to handle this, since in order
to
> > properly determine whether index.html should be added, we need to look at
the
> > filesystem.
> 
> That the nice thing about "/". For that one case, you don't need to hit the
> filesystem because there can't be an unnamed file. For "localhost/docs" that
> could be a file named "docs" or "docs/index.html". There's no easy way to
tell.
> 
> > Maybe [PubPackageProvider.loadAsset] should handle it?
> 
> That doesn't feel right to me either. From Barback's perspective,
> "docs/index.html" and "docs" should be the same asset. We wouldn't want to run
a
> transformer separately on each of those.
> 
> I added a TODO here. I think the "/" case is simple and useful enough to be
> worth keeping. I'd like to do something similar for other URLs too, but that's
> trickier.

I think supporting only "/" is actively harmful. Users will think about this in
terms of whether or not pub serve supports index.html. If they try "/" first,
they'll think it is; if they try anything else, they'll think it's not. Either
way we're setting them up to be mistaken and confused.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:91: }
Error responses should have the content-type set to text/plain.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:104: var uriBuilder = new
path.Builder(style: path.Style.url);
I think the pattern we usually use for this is a top-level final private
variable. E.g.

    final _path = new path.Builder(style: path.Style.url);

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:128: }
Currently this won't 404 for "/packages" if there's a file named "packages" in
the web directory. You can just add a TODO for that.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/pub_package_provider.dart (right):

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/pub_package_provider.dart:52: List<AssetId>
listAssets(String package, {String within}) {
Since this is now not part of the PackageProvider interface, it should be
documented here.

Also, probably don't need to support "within" anymore.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/utils.dart:32: });
You'll still need to use expect(..., completes) so that the assert here doesn't
just top-level the error.

[Bob Nystrom, 2013-07-29 21:45:43]

Thanks!

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:46:
barback.results.listen((result) {
On 2013/07/29 20:23:14, nweiz wrote:
> On 2013/07/26 20:20:11, Bob Nystrom wrote:
> > On 2013/07/25 22:58:58, nweiz wrote:
> > > Add a TODO to report errors and successful build completions via
> > > growl/notify-send if available.
> > 
> > Do we actually intend to do that? That seems painfully chatty. Builds will
run
> > in theory every time you Ctrl-S some source file.
> 
> In my experience, people like it. We can be smart about not printing it if the
> build completes fast enough, and barback will already roll together multiple
> builds into one, so if files get saved quickly in succession it won't show
> something for all of them.
> 
> If it's worth printing info on the command line, it's probably worth putting
it
> somewhere the user is likely to see as well.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:64:
HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, port).then((server) {
On 2013/07/29 20:23:14, nweiz wrote:
> On 2013/07/26 20:20:11, Bob Nystrom wrote:
> > On 2013/07/25 22:58:58, nweiz wrote:
> > > Doesn't "localhost" work these days?
> > 
> > Not sure. I figured an enum is better than a magic string. Do you have a
> > preference?
> 
> We're using "localhost" below, and "127.0.0.1" elsewhere. The constant is
> verbose and ultimately less clear than just "localhost". I'd rather use the
name
> that's more familiar.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:75: });
On 2013/07/29 20:23:14, nweiz wrote:
> On 2013/07/26 20:20:11, Bob Nystrom wrote:
> > On 2013/07/25 22:58:58, nweiz wrote:
> > > We should handle errors that come off this future.
> > 
> > Done.
> 
> Unlike errors coming off [getAssetById], errors coming from [addStream] or
> [asset.read] probably should result in a 500 error, since they're likely to be
> transient things like filesystem hiccups.

Took out the 500 stuff. There isn't any tests for it anyway.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:77: if (error is!
AssetNotFoundException) throw error;
On 2013/07/29 20:23:14, nweiz wrote:
> On 2013/07/26 20:20:11, Bob Nystrom wrote:
> > On 2013/07/25 22:58:58, nweiz wrote:
> > > This will top-level the error. That's not what we want. We should pipe
this
> to
> > > pub's error handling.
> > 
> > Done. I log it and respond with a 500. Seems about right for a dev server, I
> > think?
> 
> The only error getAssetById is expected to raise is AssetNotFoundException.
> Anything else is caused by some unexpected condition, which may involve
barback
> being in an inconsistent state -- one that may not be localized to the asset
in
> question. That should bring down the server so that it doesn't start doing bad
> things.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:89: return new
Completer().future;
On 2013/07/29 20:23:14, nweiz wrote:
> On 2013/07/26 20:20:11, Bob Nystrom wrote:
> > On 2013/07/25 22:58:58, nweiz wrote:
> > > This seems, as you say, hacky. It feels like we should return a future
that
> > will
> > > complete to any programmatic errors that occur in barback or in our
server.
> > 
> > In the above code, I'm catching all errors that come from getAssetById()
> (which
> > effectively means almost all errors) and returning a 500 and continuing.
We're
> > also logging the errors from barback's error stream. I don't think that
leaves
> > much else to report.
> 
> As with [getAssetById], programmatic errors in barback likely indicate an
> inconsistent state that should cause us to bail.

Done.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:96: if (relative == "")
relative = "index.html";
On 2013/07/29 20:23:14, nweiz wrote:
> On 2013/07/26 20:20:11, Bob Nystrom wrote:
> > On 2013/07/25 22:58:58, nweiz wrote:
> > > Supporting "index.html" only for the root seems wrong. If
> > http://localhost:8080/
> > > routes to web/index.html, then http://localhost:8080/docs should route to
> > > web/docs/index.html.
> > > 
> > > I don't know that this is the correct place to handle this, since in order
> to
> > > properly determine whether index.html should be added, we need to look at
> the
> > > filesystem.
> > 
> > That the nice thing about "/". For that one case, you don't need to hit the
> > filesystem because there can't be an unnamed file. For "localhost/docs" that
> > could be a file named "docs" or "docs/index.html". There's no easy way to
> tell.
> > 
> > > Maybe [PubPackageProvider.loadAsset] should handle it?
> > 
> > That doesn't feel right to me either. From Barback's perspective,
> > "docs/index.html" and "docs" should be the same asset. We wouldn't want to
run
> a
> > transformer separately on each of those.
> > 
> > I added a TODO here. I think the "/" case is simple and useful enough to be
> > worth keeping. I'd like to do something similar for other URLs too, but
that's
> > trickier.
> 
> I think supporting only "/" is actively harmful. Users will think about this
in
> terms of whether or not pub serve supports index.html. If they try "/" first,
> they'll think it is; if they try anything else, they'll think it's not. Either
> way we're setting them up to be mistaken and confused.

Removed it.

https://codereview.chromium.org/20204003/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:166: if (!dir.existsSync())
return;
On 2013/07/25 22:58:58, nweiz wrote:
> Use [dirExists].

Done.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:104: var uriBuilder = new
path.Builder(style: path.Style.url);
On 2013/07/29 20:23:15, nweiz wrote:
> I think the pattern we usually use for this is a top-level final private
> variable. E.g.
> 
>     final _path = new path.Builder(style: path.Style.url);

Added default builders for each style to path and changed this to use path.url.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:128: }
On 2013/07/29 20:23:15, nweiz wrote:
> Currently this won't 404 for "/packages" if there's a file named "packages" in
> the web directory. You can just add a TODO for that.

Made it do the right thing.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/pub_package_provider.dart (right):

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/pub_package_provider.dart:52: List<AssetId>
listAssets(String package, {String within}) {
On 2013/07/29 20:23:15, nweiz wrote:
> Since this is now not part of the PackageProvider interface, it should be
> documented here.
> 
> Also, probably don't need to support "within" anymore.

Done.

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/20204003/diff/10001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/utils.dart:32: });
On 2013/07/29 20:23:15, nweiz wrote:
> You'll still need to use expect(..., completes) so that the assert here
doesn't
> just top-level the error.

Done.

[nweiz, 2013-07-29 23:54:44]

A couple comments, otherwise lgtm.

https://codereview.chromium.org/20204003/diff/34001/pkg/path/lib/path.dart
File pkg/path/lib/path.dart (right):

https://codereview.chromium.org/20204003/diff/34001/pkg/path/lib/path.dart#ne...
pkg/path/lib/path.dart:386: /// [root] is omitted, it defaults ".". If *both*
[style] and [root] are
"If [root] is omitted" -> "If only [root] is omitted".

https://codereview.chromium.org/20204003/diff/34001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/34001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:90: });
If we're not doing the 500 thing for errors coming off this, at least add a TODO
to do so. If the filesystem goes briefly pear-shaped, we don't want that to
crash the server.

[Bob Nystrom, 2013-07-30 00:45:10]

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

[Bob Nystrom, 2013-07-30 01:04:27]

Thanks!

https://codereview.chromium.org/20204003/diff/34001/pkg/path/lib/path.dart
File pkg/path/lib/path.dart (right):

https://codereview.chromium.org/20204003/diff/34001/pkg/path/lib/path.dart#ne...
pkg/path/lib/path.dart:386: /// [root] is omitted, it defaults ".". If *both*
[style] and [root] are
On 2013/07/29 23:54:44, nweiz wrote:
> "If [root] is omitted" -> "If only [root] is omitted".

Done.

https://codereview.chromium.org/20204003/diff/34001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/20204003/diff/34001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/command/serve.dart:90: });
On 2013/07/29 23:54:44, nweiz wrote:
> If we're not doing the 500 thing for errors coming off this, at least add a
TODO
> to do so. If the filesystem goes briefly pear-shaped, we don't want that to
> crash the server.

Done.