First stab at a dev server in pub using barback.

sdk/lib/_internal/pub/lib/src/command/serve.dart

+// Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library pub.command.serve;
+
+import 'dart:async';
+import 'dart:io';
+
+import 'package:barback/barback.dart';
+import 'package:path/path.dart' as path;
+
+import '../command.dart';
+import '../entrypoint.dart';
+import '../exit_codes.dart' as exit_codes;
+import '../log.dart' as log;
+import '../utils.dart';
+
+final _green = getPlatformString('\u001b[32m');
+final _red = getPlatformString('\u001b[31m');
+final _none = getPlatformString('\u001b[0m');
+
+/// Handles the `serve` pub command.
+class ServeCommand extends PubCommand {
+  String get description => "Run a local web development server.";
+  String get usage => 'pub serve';
+
+  ServeCommand() {
+    commandParser.addOption('port', defaultsTo: '8080',
+        help: 'The port to listen on.');
+  }
+
+  Future onRun() {
+    return PubPackageProvider.create(entrypoint).then((provider) {
+      var port;
+      try {
+        port = int.parse(commandOptions['port']);
+      } on FormatException catch(_) {
+        log.error('Could not parse port "${commandOptions['port']}"');
+        this.printUsage();
+        exit(exit_codes.USAGE);
+      }

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

Style nit: put this in a getter to avoid the awkward "var port;" declaration.

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

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.

+
+      var barback = new Barback(provider);
+
+      barback.results.listen((result) {

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

Add a TODO to report errors and successful build completions via
growl/notify-send if available.

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

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

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

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.

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

Done.

+        if (result.succeeded) {
+          log.message("Build completed ${_green}successfully$_none");
+        } else {
+          log.message("Build completed with "
+              "${_red}${result.errors.length}$_none errors.");
+        }
+      });
+
+      barback.errors.listen((error) {
+        log.error("${_red}Build error:\n$error$_none");
+      });
+
+      // Add all of the visible files.
+      for (var package in provider.packages) {
+        barback.updateSources(provider.listAssets(package));

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

Add a TODO to re-update these sources when they change on disk.

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

Done.

+      }
+
+      HttpServer.bind(InternetAddress.LOOPBACK_IP_V4, port).then((server) {

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

Doesn't "localhost" work these days?

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

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

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

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.

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

Done.

+        log.message("Serving ${entrypoint.root.name} "
+                    "on http://localhost:${server.port}...");

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

The "..." here may make copy/pasting this URL difficult.

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

Done.

+
+        server.listen((HttpRequest request) {

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

Don't type annotate [request].

We should check that the request is GET or HEAD.

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

Done.
How should it respond to other methods? 404?

+          var id = getIdFromUri(request.uri);
+          barback.getAssetById(id).then((asset) {
+            log.message(
+                "$_green${request.method}$_none ${request.uri} -> $asset");
+            request.response.addStream(asset.read()).then((_) {

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

Add a TODO to set the Content-Type based on the extension.

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

Done.

+              request.response.close();

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

I thought [addStream] automatically closed the consumer when the stream passed
in was closed. I may be wrong, though.

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

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."

+            });

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

We should handle errors that come off this future.

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

Done.

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

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.

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

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

+          }).catchError((error) {
+            if (error is! AssetNotFoundException) throw error;

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

This will top-level the error. That's not what we want. We should pipe this to
pub's error handling.

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

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

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

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.

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

Done.

+            log.error("$_red${request.method}$_none ${request.uri} -> $error");
+            request.response.statusCode = 404;
+            request.response.reasonPhrase = error.toString();

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

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.

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

Done.

+            request.response.close();
+            return;

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

Remove.

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

Done.

+          });
+        });
+      });
+
+      // TODO(rnystrom): Hack! Return a future that never completes to leave
+      // pub running.
+      return new Completer().future;

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

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.

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

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.

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

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

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

Done.

+    });
+  }
+
+  AssetId getIdFromUri(Uri uri) {
+    // Strip the leading "/" from the URL.
+    var relative = uri.path.substring(1);

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

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.

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

Done.

+    if (relative == "") relative = "index.html";

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

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?

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

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.
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.

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

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.

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

Removed it.

+
+    var parts = path.split(relative);

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

This will break on windows. You need to make a URL-specific Builder.

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

Done.

+
+    // Checks to see if [uri]'s path contains a special "symlink" directory
+    // named [name]. If so, maps the path after that to be within [dir].

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

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].

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

Done.

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

Done.

+    AssetId _trySpecialUrl(String name, String dir) {
+      // Find the package name and the relative path in the package.
+      var index = parts.indexOf(name);
+      if (index == -1) return null;
+
+      var package = parts[index + 1];
+      var assetPath = path.join(dir, path.joinAll(parts.skip(index + 2)));
+      return new AssetId(package, assetPath);

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

Right now this will crash on paths like "packages" or "packages/foo". It should
do something that leads to a 404 in those cases.

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

Done.

+    }
+
+    // See if it's "packages" URL.
+    var id = _trySpecialUrl("packages", "lib");
+    if (id != null) return id;
+
+    // See if it's an "assets" URL.
+    id = _trySpecialUrl("assets", "asset");
+    if (id != null) return id;
+
+    // Otherwise, it's a path in current package's web directory.
+    relative = path.join("web", relative);
+    return new AssetId(entrypoint.root.name, relative);
+  }
+}
+
+class PubPackageProvider implements PackageProvider {

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

This is going to be used elsewhere pretty soon. Might as well give it its own
library now.

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

Done.

+  /// The [Entrypoint] package being served.
+  final Entrypoint _entrypoint;
+
+  /// Maps the names of all of the packages in [_entrypoint]'s transitive
+  /// dependency graph to the local path of the directory for that package.
+  final Map<String, String> _packageDirs;
+
+  /// Creates a new provider for [entrypoint].
+  static Future<PubPackageProvider> create(Entrypoint entrypoint) {
+    var packageDirs = <String, String>{};
+    var futures = [];
+
+    packageDirs[entrypoint.root.name] = entrypoint.root.dir;
+
+    // Cache package directories up front so we can have synchronous access
+    // to them.
+    // TODO(rnystrom): Handle missing or out of date lockfile.
+    entrypoint.loadLockFile().packages.forEach((name, package) {
+      var source = entrypoint.cache.sources[package.source];
+      futures.add(source.getDirectory(package).then((packageDir) {
+        packageDirs[name] = packageDir;
+      }));
+    });

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

Rather than manually expanding it, just use
`entrypoint.loadLockFile().packages.map`.

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

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

+
+    return Future.wait(futures).then((_) {
+      return new PubPackageProvider._(entrypoint, packageDirs);
+    });
+  }
+
+  PubPackageProvider._(this._entrypoint, this._packageDirs);
+
+  Iterable<String> get packages => _packageDirs.keys;
+
+  List<AssetId> listAssets(String package, {String within}) {

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

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.

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

It was being used forever ago when I was prototyping plug-in loading.
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.

+    var files = <AssetId>[];
+
+    addFiles(String dirPath) {
+      var packageDir = _packageDirs[package];
+      var dir = new Directory(path.join(packageDir, dirPath));
+      if (!dir.existsSync()) return;

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

Use [dirExists].

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

Done.

+      var entries = dir.listSync(recursive: true);

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

Use [listDir] rather than [Directory.listSync].

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

Done.

+      files.addAll(entries
+          .where((entry) => entry is File)
+          .map((entry) => new AssetId(package,
+              path.relative(entry.path, from: packageDir))));
+    }
+
+    // If we aren't given a subdirectory, use all externally visible files.
+    if (within == null) {
+      // Expose the "asset" and "lib" directories.
+      addFiles("asset");
+      addFiles("lib");
+
+      // The entrypoint's "web" directory is also visible.
+      if (package == _entrypoint.root.name) {
+        addFiles("web");
+      }
+    } else {
+      addFiles(within);
+    }
+
+    return files;
+  }

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

This shouldn't list the contents of packages directories.

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

Done.

+
+  // TODO(rnystrom): Actually support transformers.
+  Iterable<Iterable<Transformer>> getTransformers(String package) => [];
+
+  Future<Asset> getAsset(AssetId id) {
+    var file = path.normalize(path.join(_packageDirs[id.package], id.path));

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

What's the value of normalizing the path here?

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

Not much. Removed.

+    return new Future.value(new Asset.fromPath(id, file));
+  }
+}