First pass at build dependency graph for barback.

pkg/barback/lib/barback.dart

+// Copyright (c) 2013, 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 barback;
+
+import 'dart:async';
+import 'dart:io';
+
+// TODO(rnystrom): Is this the prefix we want to use?

[nweiz, 2013/06/14 00:57:57]

yes

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+import 'package:pathos/path.dart' as pathos;
+
+/// Identifies some asset within a package.
+class AssetId {

[nweiz, 2013/06/14 00:57:57]

I'd rather see each of these classes in its own library that gets exported here.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+  /// Parses an [AssetId] string of the form "package|path/to/asset.txt".

[nweiz, 2013/06/14 00:57:57]

Why "|"? Why not ":"?

[Bob Nystrom, 2013/06/17 23:35:05]

I did ":" at first, but I worried that it makes asset IDs look like URLs with
random schemes, which they are definitely not. This makes it clearer that they
are different, and gives them a distinct look. At a glance you can see it's a
package ID and not a path or URL.

[nweiz, 2013/06/18 23:14:45]

I've found the look of these IDs very odd. I was expecting something like
"package:" URIs as IDs for assets in "lib" directories, and "asset:" URIs as IDs
for assets in the "asset" directories. That seems like it would have less
conceptual overhead -- instead of teaching people an entirely new syntax for
identifying assets, we piggyback on the syntax they understand. That also has
the benefit of not requiring people to type "lib" or "asset" all over the place.

On the other hand, I suppose that poses a problem if people want to do
transformations on files in `web` or `bin`. Another possibility would be
"asset:package/path/to/asset.txt", which mirrors "package:" somewhat but
provides access to all files in a package.

[Bob Nystrom, 2013/06/20 00:23:59]

At least right now, this method is only really being used by tests. (At one
point, it was just a function in the test utils code.) I wasn't intending this
to be a very user-visible syntax for assets.

I realize that we do need that syntax for things like referencing assets in
non-Dart files, but it wasn't my overt intent to use this for that. I just
needed something terse so that I could create ids in tests and print them to
debug my code.

I do kind of like the way these ones look, but I'm up for iterating on them in
later patches. This definitely isn't carved in stone.

[nweiz, 2013/06/20 23:06:08]

Okay, sounds good.

+  static AssetId parse(String description) {

[nweiz, 2013/06/14 00:57:57]

Why isn't this a constructor? I thought we decided that it didn't make sense to
separate out "parse" methods as statics.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+    var parts = description.split("|");

[nweiz, 2013/06/14 00:57:57]

Assert that there's only two parts here.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+    return new AssetId(parts[0], parts[1]);
+  }
+
+  final String package;
+  final String path;
+
+  String get extension => pathos.extension(path);
+
+  AssetId(this.package, String path)
+      : path = pathos.normalize(path);

[nweiz, 2013/06/14 00:57:57]

This will break on Windows, since the asset paths are presumably all going to be
POSIX-style. We should store a POSIX-style builder and use that instead.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+
+  AssetId.deserialize(data)
+      : package = data[0],
+        path = data[1];
+
+  operator ==(other) {
+    if (other is! AssetId) return false;

[nweiz, 2013/06/14 00:57:57]

Style nit: fold this into the returned expression.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+    return package == other.package &&
+           path == other.path;
+  }
+
+  int get hashCode => package.hashCode ^ path.hashCode;
+
+  AssetId addExtension(String extension) {
+    return new AssetId(package, "$path.$extension");

[nweiz, 2013/06/14 00:57:57]

Style nit: =>

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+  }
+
+  AssetId changeExtension(String newExtension) {
+    var newPath = path.substring(0, path.length - extension.length) +

[nweiz, 2013/06/14 00:57:57]

pathos.withoutExtension

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+        newExtension;
+    return new AssetId(package, newPath);
+  }
+
+  String toString() => "$package|$path";
+
+  serialize() => [package, path];
+}

[nweiz, 2013/06/14 00:57:57]

All these members will need documentation.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+
+/// An identifiable blob of data. Assets may come from the file system, or

[nweiz, 2013/06/14 00:57:57]

Clarify what "identifiable" means.

Also, move the second sentence into its own paragraph. Same goes for several
other doc comments.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

[nweiz, 2013/06/18 23:14:45]

Most of these comments still have more than one sentence in their initial
paragraphs. This is contrary to the documentation style guide.

[Bob Nystrom, 2013/06/20 00:23:59]

Done.

+/// from some generation process.
+abstract class Asset {
+  Asset();

[nweiz, 2013/06/14 00:57:57]

Why does this class have a constructor? It seems like it would work fine as an
interface; there's no behavior we get from extending it.

[Bob Nystrom, 2013/06/17 23:35:05]

Because it has other named constructors, it doesn't get an automatic unnamed
constructor. That means the subclasses _FileAsset and _StringAsset don't have a
superclass constructor to invoke if this is omitted.

[nweiz, 2013/06/18 23:14:45]

But why have subclasses at all? Why not just make it an interface?

[Bob Nystrom, 2013/06/20 00:23:59]

Oh, duh. Right. I think at some point there used to be a concrete inherited
method or two but not anymore. Done.

+
+  factory Asset.fromFile(File file) {
+    return new _FileAsset(file);
+  }
+
+  factory Asset.fromString(String content) {
+    return new _StringAsset(content);
+  }
+
+  factory Asset.fromPath(String path) {
+    return new _FileAsset(new File(path));
+  }
+
+  factory Asset.deserialize(data) {

[nweiz, 2013/06/14 00:57:57]

The rest of this class makes me think that Asset can be implemented outside of
the barback package, but this constructor will break if users do that.

Maybe the thing to do is to use the serialization library so we can legitimately
support external subclasses of this class.

[Bob Nystrom, 2013/06/17 23:35:05]

That is a use case I had in mind.
Good point.
I'll add a TODO. This is here for the plug-in stuff which is still being
implemented. Once that becomes "real" code, using serialization is probably the
right thing to do.

+    // TODO(rnystrom): Handle errors.
+    switch (data[0]) {
+      case "file": return new _FileAsset(new File(data[1])); break;
+      case "string": return new _StringAsset(data[1]); break;
+    }
+  }
+
+  String readAsString();

[nweiz, 2013/06/14 00:57:57]

This method seems weird to me. Why do the assets need to implement it if they
also implement [read]? Why is it synchronous when [read] is asynchronous? What
does it do with binary-only assets?

[Bob Nystrom, 2013/06/17 23:35:05]

Convenience.
Convenience.
Good question. I've got some half-formed ideas here, but nothing fleshed out
yet. I'll add a TODO.

+  Stream<List<int>> read();
+
+  serialize();
+}
+
+/// An asset backed by a file on the local file system.
+class _FileAsset extends Asset {
+  final File _file;

[nweiz, 2013/06/14 00:57:57]

I don't like storing a [File] object. I like the pub style of treating the
[File] APIs as weird-looking static methods.

[Bob Nystrom, 2013/06/17 23:35:05]

File is just a wrapper around a path. If I don't store it, each method will just
create one every time and throw it away. Seemed cleaner this way.

[nweiz, 2013/06/18 23:14:45]

I prefer creating a new [File] each time; I feel like it makes it clearer that
it doesn't have any state other than the path itself. I don't feel especially
strongly about it, though.

[Bob Nystrom, 2013/06/20 00:23:59]

I kind of like it this way. I'll leave it for now but I'm up for changing it if
it gets unwieldy.

+  _FileAsset(this._file);
+
+  String readAsString() => _file.readAsStringSync();
+  Stream<List<int>> read() => _file.openRead();
+
+  String toString() => 'File "${_file.path}"';
+
+  Object serialize() => ["file", _file.path];
+}
+
+/// An asset whose data is stored in a string.
+class _StringAsset extends Asset {

[nweiz, 2013/06/14 00:57:57]

What about in-memory binary assets? It seems like it might be better to just
have an in-memory asset class that stores the asset as a byte array, and have a
convenience method on Asset that gets the string version if the user knows it's
text.

[Bob Nystrom, 2013/06/17 23:35:05]

Yeah, I don't have any real support for binary assets yet. It may be worth
having StringAsset for text assets because they may be faster/more compact in
the VM. But having something for in-memory binary assets will be good too. Added
a TODO.

+  final String _contents;
+
+  _StringAsset(this._contents);
+
+  String readAsString() => _contents;
+  // TODO(rnystrom): Handle encoding?

[nweiz, 2013/06/14 00:57:57]

Absolutely!

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+  Stream<List<int>> read() => new Stream<List<int>>.fromIterable([_contents.codeUnits]);

[nweiz, 2013/06/14 00:57:57]

This is broken for a lot of non-ASCII text. [codeUnits] will return numbers >=
256, which will choke anything that expects a byte stream.

Also, long line.

[Bob Nystrom, 2013/06/17 23:35:05]

This code path isn't being used or tested yet anyway, so just changed it to
throw UnimplementedError for now and added a TODO. I'll make it do something
less done when I actually implement. :)

+
+  String toString() {
+    // Don't show the whole string if it's long.
+    var contents = _contents;
+    if (contents.length > 40) {
+      contents = contents.substring(0, 20) + " ... " +
+                 contents.substring(contents.length - 20);
+    }
+
+    contents = _escape(contents);
+    return 'String "$contents"';
+  }
+
+  Object serialize() => ["string", _contents];
+
+  String _escape(String string) {
+    return string
+        .replaceAll("\"", r'\"')
+        .replaceAll("\n", r"\n")
+        .replaceAll("\r", r"\r")
+        .replaceAll("\t", r"\t");
+  }
+}
+
+/// API for locating and accessing packages on disc. Implemented by pub and

[nweiz, 2013/06/14 00:57:57]

"disc" -> "disk"

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+/// provided to barback so that it isn't coupled directly to pub.
+abstract class AssetProvider {
+  /// The names of all packages that can be provided by this provider. This will be the

[nweiz, 2013/06/14 00:57:57]

Long line.

[Bob Nystrom, 2013/06/17 23:35:05]

Done.

+  /// transitive dependency graph of the entrypoint package.
+  Iterable<String> get packages;
+
+  // TODO(rnystrom): Make this async.
+  /// The paths of all available asset files in [package], relative to the
+  /// package's root directory.
+  ///
+  /// You can pass [within], which should be the relative path to a directory
+  /// within the package, to only return the files within that subdirectory.

[nweiz, 2013/06/14 00:57:57]

Does this give access to all files in the package, or just those under an
"asset" directory? This distinction will need to be clarified before we ship
this.

[Bob Nystrom, 2013/06/17 23:35:05]

All files. This lets it access stuff in asset/ but also stuff in web/ for the
entrypoint package and lib/ since Dart files are assets too.

At least right now, barback is actually pretty pub-agnostic. I'm not sure if
that's a feature or just a coincidence of the current implementation and the
lack of an actual Pub API. It does make it much easier to unit test. :)

[nweiz, 2013/06/18 23:14:45]

My intuition is that we'll eventually have to decide whether to keep it mostly
Pub-agnostic or not; I suspect "not" is the correct direction, since
approximately everyone will be using it with Pub anyway.

[Bob Nystrom, 2013/06/20 00:23:59]

Agreed. I don't intend to try hard to keep it untainted by pub. I just want it
loosely coupled enough to be able to test easily.

+  List<String> listFiles(String package, {String within});

[nweiz, 2013/06/14 00:57:57]

It seems like this should be "listAssets".

[Bob Nystrom, 2013/06/17 23:35:05]

Done. Made it return AssetIds too.

+
+  Future<Asset> loadAsset(AssetId id);

[nweiz, 2013/06/14 00:57:57]

This name is weird, since it doesn't actually load the asset from disk for
filesystem assets (which I assume is what's going to be loaded when calling into
pub). It effectively just returns a path to the asset.

[Bob Nystrom, 2013/06/17 23:35:05]

Changed to getAsset() to be a bit more oblique. From the user's perspective it
does "load" it in the sense that they get an object back that they can read
from. It is true that it doesn't cache the contents of the file in memory at the
point in time that you call this. I'm not sure if that's behavior that should be
specified one way or the other.

+}
+
+/// Error thrown when an asset with [id] cannot be found.
+class AssetNotFoundException implements Exception {
+  final AssetId id;
+
+  AssetNotFoundException(this.id);
+
+  String toString() => "Could not find asset $id.";
+}
+
+/// Error thrown when two transformers both output an asset with [id].
+class AssetCollisionException implements Exception {
+  final AssetId id;
+
+  AssetCollisionException(this.id);
+
+  String toString() => "Got collision on asset $id.";
+}
+
+/// Error thrown when a transformer requests an input [id] which cannot be
+/// found.
+class MissingInputException implements Exception {
+  final AssetId id;
+
+  MissingInputException(this.id);
+
+  String toString() => "Missing input $id.";
+}