First pass at build dependency graph for barback.

pkg/barback/lib/src/asset_graph.dart

Index: pkg/barback/lib/src/asset_graph.dart
diff --git a/pkg/barback/lib/src/asset_graph.dart b/pkg/barback/lib/src/asset_graph.dart
new file mode 100644
index 0000000000000000000000000000000000000000..61a5bc769013d827ceed3a598798f56c87d0aebc
--- /dev/null
+++ b/pkg/barback/lib/src/asset_graph.dart
@@ -0,0 +1,404 @@
+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file

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

This should probably be exported somewhere, right?

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

The Barback class (when it exists) will wrap it and use it internally, but it
won't be publicly exposed. Think of it like VersionSolver for pub.

(Depending on how much code outside of this that barback ends up having, I may
just end up unifying it with that class.)

+// 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.asset_graph;
+
+import 'dart:async';
+import 'dart:collection';
+
+import '../barback.dart';
+import '../transformer.dart';
+
+/// The main build dependency manager. For any given input file, it can tell
+/// which output files are affected by it, and vice versa.
+class AssetGraph {
+  final AssetProvider _provider;
+
+  final _phases = <_Phase>[];
+
+  // TODO(rnystrom): Have a value type for results.
+  Stream<ProcessResult> get results => _resultsController.stream;
+  final _resultsController = new StreamController<ProcessResult>();

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

This should be a broadcast controller.

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

Done.

+
+  /// This holds a future that completes when the work queue is complete if the
+  /// /work queue is currently being processed. Otherwise, it is `null`.

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

"/work" -> "work"

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

Done.

+  Future _processDone;
+
+  _ChangeBatch _sourceChanges;
+
+  /// Creates a new [AssetGraph].
+  ///
+  /// It loads source assets using [provider] and then uses [transformerPhases]
+  /// to generate output files from them.
+  //TODO(rnystrom): Better way of specifying transformers and their ordering.
+  AssetGraph(this._provider, Iterable<Iterable<Transformer>> transformerPhases) {

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

Long line.

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

Done.

+    // Add phases for each transformer stage.

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

I'm convinced we're going to end up needing semantically-distinct phases.

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

I'm not convinced either way, but so far I haven't needed them in the
implementation. I'd like to beef up the test suite and see if I can find graphs
where I do need semantically-meaningful phases to get the behavior I intuit.

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

The biggest driver won't come when you're doing the implementation, it'll come
when we're doing the UI. The implementation as it stands relies on receiving a
set of transformers that are pre-divided into phases. It'll be unduly onerous to
require that users manually divide their transformers into phases, and
impossible to have the importers try to organize themselves in a generic way.

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

Sure, but my rough thinking is that that can happen at a layer above AssetGraph.
In fact, that may be one of the main things the soon-to-be Barback class does.

What matters is that at some point, the set of Transformers can be organized
into explicit phases and that those don't change while an AssetGraph is in use.
If you accept that given, then AssetGraph can just require that as an explicit
input and can leave the work to determine that to other code.

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

Keep in mind that we may eventually want to support adding or removing
additional transformers to a running graph. Once users get used to an
edit/refresh cycle, it will seem weird that that doesn't work for their
transformers.

[Bob Nystrom, 2013/06/21 00:13:20]

Agreed. I'm making the simplifying assumption for now that the set of phases and
transformers is fixed, but it wouldn't be a huge change to allow that to mutate.

+    for (var transformers in transformerPhases) {
+      var phase = new _Phase(this, _phases.length, transformers.toList());
+      _phases.add(phase);
+    }
+
+    // Add a phase for the final outputs.
+    _phases.add(new _Phase(this, _phases.length, []));

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

This is confusing. What does a phase with no transformers do? Add some more
explanation.

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

Done.

+
+    // Chain them together.
+    for (var i = 0; i < _phases.length - 1; i++){
+      _phases[i].next = _phases[i + 1];
+    }
+  }
+
+  Future<Asset> getAssetById(AssetId id) {
+    return _waitForProcess().then((_) {

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

Add a TODO to be smarter about only waiting until the asset we need is
generated.

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

At least right now, this is as smart as it can be. It has to wait until all
phases are complete because any dirty transform could start outputting the
requested asset regardless of whether it did the last time it was run.

What this *will* do, though, is only run transforms whose *inputs* are dirty, so
running through all of the phases should fast since only modified transforms get
run. In other words, we prune the graph by only traversing from modified inputs,
not by only traversing from requested outputs.

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

There are several ways this could be smarter:

* [id] may be generated before the compilation is finished. We should be able to
quickly check whether there are any more in-place transformations that can be
run on it. If not, we can return it early.

* If everything is compiled, something that didn't output [id] is dirtied, and
then [id] is requested, we can return it immediately, since anything overwriting
it at that point is an error.

* If [id] has never been generated and all active transformers provide metadata
about the file names of assets it can emit, we can prove that none of them can
emit [id] and fail early.

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

All good points. Added a big TODO with all of this. These are definitely the
kind of optimizations I'd like to add as it matures.

+      // Find the latest phase that output this asset.
+      for (var i = _phases.length - 1; i >= 0; i--) {
+        var node = _phases[i].inputs[id];

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

Why are we looking in the phase inputs for the asset output?

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

Generated assets are stored in each phase. If you have:

sources -> [phase 1] -> intermediate -> [phase 2] -> outputs

You'll have three instances of _Phase:

The first has sources as its .inputs and has transforms for [phase 1].
The second has intermediate as its .intputs and has transforms for [phase 2].
The third has outputs as its .inputs and no transforms.

At one point, I had a separate class for an "asset phase" in the graph but it
just seemed to make things more complex.

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

This would be clearer if the comment said "Find the latest phase that takes this
asset as an input", and also explained how the final phase was special.

Also, if I'm reading this correctly, if you have a rename transformer you'll
still be able to access the asset by its old name. This seems wrong to me. Once
an asset has been transformed, it should be unavailable afterwards.

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

Done.
That's correct. I think we'll do something related to this but I'm not sure what
just yet. Added a TODO explaining this. Once things like the pub deploy
integration are farther along, I'll have a clearer picture of what the semantics
should be.

+        if (node != null) {
+          // By the time we get here, the asset should have been built.
+          assert(node.asset != null);
+          return node.asset;
+        }
+      }
+
+      // Couldn't find it.
+      _throwError(new AssetNotFoundException(id));
+    });
+  }
+
+  /// Adds [sources] to the graph's known set of source assets. Will begin

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

"Will begin" -> "Begins"

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

Done.

+  /// applying any transforms that can consume any of the sources. If a given
+  /// source has already been added, it is considered modified and all

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

"has already been added" -> "is already known"

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

Done.

+  /// transforms that use it will be re-applied.
+  void updateSources(Iterable<AssetId> sources) {

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

Right now I think you can postpone handling a change indefinitely. If a change
comes in while you're processing the last phase of the previous batch, then once
that phase finishes processing no further changes come in, nothing will trigger
processing the next batch of changes.

Add a test for that case as well.

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

Added a test. I think it's working correctly here, but it may be the test isn't
right. When the a batch is first processed, it gets cleared before any phases
start. When a change happens during the last phase, it gets added to a new batch
since the last batch is gone. When that phase is complete, it loops by calling
_process() which always checks to see if there is a changed batch first.

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

I was misreading somewhat, but I think there's still a small possibility that
this can happen. Suppose everything's finished running. Then [updateSources] is
called with a source that, for whatever reason, doesn't dirty any transforms.
[_processSourceChanges] will run. While the source is being loaded,
[updateSources] is called again with a source that *does* dirty some transforms.

Now the call to [_processSourceChanges] finishes loading the first source and
passes it to [_phases.first.updateInputs]. Then [_process] runs
[_phases.first.process]. Since the first source change didn't dirty anything,
[_phases.first.process] returns null. This causes [_process] to exit before
recursively calling itself, meaning that the second change is now waiting to be
run forever.

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

Wow, good catch. One line fix, but took me a good while to build a test that
could actually catch this. Done.

+    if (_sourceChanges == null) _sourceChanges = new _ChangeBatch();
+    _sourceChanges.update(sources);
+
+    _waitForProcess();
+  }
+
+  /// Removes [removed] from the graph's known set of source assets.
+  void removeSources(Iterable<AssetId> removed) {
+    if (_sourceChanges == null) _sourceChanges = new _ChangeBatch();
+    _sourceChanges.remove(removed);
+
+    _waitForProcess();
+  }
+
+  /// Returns a future that completes with the background processing is done.

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

It's not clear from the name or from the documentation that this kicks off a
process if one hasn't been started yet. This makes the calls to
_waitForProcess() in the previous two methods confusing, since nothing consumes
the Future.

Also, using this to start processing without adding any handlers is dangerous;
if an error occurs before someone calls [getAssetById], the error will be
top-leveled. You should have a test that exercises this behavior.

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

Clarified documentation.
Added some tests. It was always intended that the future returned by this will
not throw any errors so you can discard it. Now it's better about actually doing
that, and there's some tests of this.

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

I still worry that unintended errors from e.g. [_provider.getAsset] will get
here and crash the program in an unhandlable way. I just don't think we should
ever have dangling futures without a proximate [catchError].

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

My intent (and there are TODOs for this) is to catch all errors coming from both
of the AssetProvider and Transformer APIs. That should catch cases where errors
leak in from external code. Other unexpected errors are programmatic errors in
barback itself and can take it down.

It does look weird here, but I think it makes sense given its "run in the
background" design.

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

Taking down barback as a component and taking down the entire process
unrecoverably are different things. You should pipe errors from these futures to
the results stream (as actual stream errors, not error results) so that users of
this code can handle or ignore them if they want. Letting errors top-level is
never the correct behavior for a library; it's as bad as calling [exit].

[Bob Nystrom, 2013/06/21 00:13:20]

Good call. Done. Added a long comment explaining it.

+  Future _waitForProcess() {
+    if (_processDone != null) return _processDone;
+    // TODO(rnystrom): Handle errors.
+    return _processDone = _process().then((_) {
+      _processDone = null;
+    });
+  }
+
+  /// Starts the background processing. Returns a future that completes when
+  /// all assets have been processed.
+  Future _process() {
+    return _processSourceChanges().then((_) {
+      // Find the first phase that has work to do and do it.
+      var future;
+      for (var phase in _phases) {
+        future = phase.process();
+        if (future != null) break;
+      }
+
+      // If all phases are done, so are we.
+      if (future == null) return;
+
+      // Process that phase and then loop onto the next.
+      return future.then((_) => _process());
+    });
+  }
+
+  /// Processes the current batch of changes to source assets.
+  Future _processSourceChanges() {
+    if (_sourceChanges == null) return new Future.value();
+
+    // Always pump the event loop. This ensures a bunch of synchronous source
+    // changes are processed in a single batch even when the first one starts
+    // the build process.

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

I don't think this works right now. You'll end up with a bunch of callbacks
queued up, each of which will run "changes = _sourceChanges; _sourceChanges =
null". Then all but the first one will have a null "changes" and break.

This warrants a test.

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

It's very unclear, but the behavior is correct. It isn't possible to enqueue a
bunch of callbacks because _waitForProcess() won't get here if the process
future already exists. This code isn't very helpful at making that clear,
though. Moving the if check into the future to make it more obvious.
There's a test that multiple calls to updateSources() create a single batch.

+    return new Future(() {
+      var changes = _sourceChanges;
+      _sourceChanges = null;
+
+      var updated = new Map<AssetId, Asset>();
+      var futures = [];
+      for (var id in changes.updated) {
+        futures.add(_provider.loadAsset(id).then((asset) {
+          updated[id] = asset;
+        }));
+      }
+
+      return Future.wait(futures).then((_) {
+        _phases.first.updateInputs(updated, changes.removed);
+      });
+    });
+  }
+
+  /// Reports a process result with the given error then throws it.
+  void _throwError(Exception error) {

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

It feels like it might be cleaner to just throw exceptions normally, and report
them to _resultsController from some catch. That also avoids having external
classes call [_throwError] on the AssetGraph, which is kind of icky.

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

Removed this.

+    _resultsController.add(new ProcessResult(error));
+    throw error;
+  }
+}
+
+/// The build process runs asynchronously in the background. It reports back to
+/// the user be emitting a [Stream] of these objects. Currently, it only emits
+/// errors.
+class ProcessResult {
+  /// The error that occurred.
+  final error;
+
+  ProcessResult(this.error);
+}
+
+/// Represents a batch of source asset changes: additions, removals and
+/// modifications.
+class _ChangeBatch {

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

It would be nice to split out this and the following classes into separate files
as well.

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

Done. For some reason, I thought they should be together so they could be
private, but all of AssetGraph is hidden from the public API, so there's no
reason not to split these out into other private libraries.

+  /// The assets that have been added or modified in this batch.
+  final updated = new Set<AssetId>();
+
+  /// The assets that have been removed in this batch.
+  final removed = new Set<AssetId>();
+
+  /// Adds the updated [assets] to this batch.
+  void update(Iterable<AssetId> assets) {
+    updated.addAll(assets);
+
+    // If they were previously removed, they are back now.
+    removed.removeAll(assets);
+  }
+
+  /// Removes [assets] from this batch.
+  void remove(Iterable<AssetId> assets) {
+    removed.addAll(assets);
+
+    // If they were previously updated, they are gone now.
+    updated.removeAll(assets);
+  }
+}
+
+/// The transforms in a processing graph are organized into a series of phases.
+/// Each phase can access outputs from previous phases and can in turn pass
+/// outputs to later phases.
+///
+/// Phases are processed strictly serially. All transforms in a phase will be
+/// complete before moving on to the next phase. Within a single phase, all
+/// transforms will be run in parallel.
+///
+/// Building can be interrupted between phases. For example, an source is added
+/// which starts the background process. Sometime during phase 2 (which is
+/// running asynchronously) that source is modified. When the process queue
+/// goes to advance to phase 3, it will see that modification and start the
+/// waterfall from the beginning again.
+class _Phase {
+  /// The graph that owns this phase.
+  final AssetGraph graph;
+
+  /// This phase's position relative to the other phases. Zero-based.
+  final int index;
+
+  /// The transformers that can use [assets] as inputs. Their outputs will be

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

There's no field named "assets". Did you mean "inputs"?

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

Done.

+  /// available to the next phase.
+  final List<Transformer> transformers;
+
+  /// The inputs that are available for transforms in this phase to consume.
+  /// For the first phase, these will be the source assets. For all other
+  /// phases, they will be the outputs from the previous phase.
+  final inputs = new Map<AssetId, _AssetNode>();
+
+  /// The transforms currently applicable on assets in [inputs]. These are the

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

"applicable to"

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

Done.

+  /// transforms that have been "wired up": they represent a repeatable
+  /// transformation of a single concrete set of inputs. "dart2js" is a
+  /// transformer. "dart2js on web/main.dart" is a transform.
+  final transforms = new Set<_TransformNode>();
+
+  /// The nodes that are new in the graph since the last time [process] was
+  /// called. When we process, we'll check these to see if we can hang new
+  /// transforms off them.

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

This isn't all new nodes in the whole AssetGraph, is it? Isn't it just the nodes
that have been added to [inputs]?

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

Yes, fixed.

+  final newInputs = new Set<_AssetNode>();
+
+  /// The phase after this one. Outputs from this phase will be passed to it.
+  _Phase next;
+
+  _Phase(this.graph, this.index, this.transformers);
+
+  /// Updates the phase's inputs with [updated] and removes [removed]. This
+  /// marks any affected [transforms] as dirty or discards them if their inputs
+  /// are removed.
+  void updateInputs(Map<AssetId, Asset> updated, Set<AssetId> removed) {

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

It's weird that this takes [updated] as a map. It feels like an Asset should
know its id.

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

They did at first. It made some things cleaner to not have it in there, though
it may not make much of a difference now. I kind of like it how it is, but I'm
open to changing it, preferably later.

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

Let's postpone the decision until we have a better idea of how Asset and AssetId
are used in the system as a whole.

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

SGTM.

+    // Remove any nodes that are no longer being output.
+    for (var id in removed) {
+      var node = inputs.remove(id);
+
+      // Every transform that was using it is dirty now.
+      if (node != null) {
+        node.consumers.forEach((consumer) => consumer.isDirty = true);
+      }
+    }
+
+    // Update and new or modified assets.
+    updated.forEach((id, asset) {
+      var node = inputs.putIfAbsent(id, () => new _AssetNode(id));
+
+      // If it's a new node, remember that so we can see if any new transforms
+      // will consume it.
+      if (node.asset == null) newInputs.add(node);
+
+      node.updateAsset(asset);
+    });
+  }
+
+  /// Processes this phase. For all new inputs, it tries to see if there are
+  /// transformers that can consume them. Then all applicable transforms are
+  /// applied.
+  ///
+  /// Returns a future that completes when processing is done. If there is
+  /// nothing to process, returns `null`.
+  Future process() {
+    var future = _processNewInputs();
+    if (future == null) {
+      return _processTransforms();

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

It'd be cleaner to just do "future = new Future.value()" here. The way it is now
is awkward if the call to _processTransforms ever gets more complicated.

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

I hate how awkard this code is, but it's the best I could come up with. They key
bit is that it needs to return null if both _processNewInputs() and
_processTransforms() return null. If I do future = new Future.value() here, it
won't do that.

+    }
+
+    return future.then((_) {
+      return _processTransforms();

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

Style nit: =>

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

Done.

+    });
+  }
+
+  /// Creates new transforms for any new inputs that are applicable.
+  Future _processNewInputs() {
+    if (newInputs.isEmpty) return null;
+
+    var futures = [];
+    for (var node in newInputs) {
+      for (var transformer in transformers) {
+        futures.add(transformer.isPrimary(node.id).then((isPrimary) {
+          if (!isPrimary) return;
+          var transform = new _TransformNode(this, transformer, node);
+          node.consumers.add(transform);
+          transforms.add(transform);
+        }));
+      }
+    }
+
+    newInputs.clear();
+
+    return Future.wait(futures);
+  }
+
+  /// Applies all currently wired up and dirty transforms. Passes their outputs
+  /// to the next phase.
+  Future _processTransforms() {
+    var dirtyTransforms = transforms.where((transform) => transform.isDirty);
+    if (dirtyTransforms.isEmpty) return null;
+
+    var updated = new Map<AssetId, Asset>();
+    var removed = new Set<AssetId>();
+
+    return Future.wait(dirtyTransforms.map((node) {

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

"node" -> "transform", to avoid it being confused with an AssetNode.

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

Done.

+      return node.apply(updated, removed);
+    })).then((_) {
+      // Pass the outputs to the next phase.
+      next.updateInputs(updated, removed);

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

What about the transform's output? Isn't it also updated?

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

The input/output terminology is a bit confusing here. It's updating the inputs
of the *next* phase, which are the *outputs* of this one.

+    });
+  }
+}
+
+/// Represents an asset within the build dependency graph. It tracks its ID,

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

"Represents an asset" is confusing. The Asset class already represents an asset.

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

Rewrote.

+/// the currently generated actual asset for it, and any transforms that use

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

You're using "it" to refer to two different things in this sentence.

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

Done.

+/// that asset as an input.
+class _AssetNode {
+  final AssetId id;
+  Asset asset;
+
+  /// The [_TransformNode]s in this node's phase that consume this asset as an

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

"this asset" -> "this node's asset".

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

Done.

+  /// input.
+  final consumers = new Set<_TransformNode>();
+
+  _AssetNode(this.id);
+
+  /// Updates this nodes's generated asset value and marks all transforms that

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

"node's"

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

Done.

+  /// use this as dirty.
+  void updateAsset(Asset asset) {
+    this.asset = asset;
+    consumers.forEach((consumer) => consumer.isDirty = true);
+  }
+}
+
+/// Represents a transform step within the build dependency graph.
+class _TransformNode {

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

It's pretty confusing right now what the distinction is between this and
Transform. Document that more thoroughly.

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

Done.

+  final _Phase phase;
+  final Transformer transformer;
+  final _AssetNode primary;
+  var isDirty = true;
+
+  /// The outputs created by this transform the last time it was run. Used to
+  /// tell if an output was removed in a later run.
+  var outputs = new Set<AssetId>();

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

What about the non-primary inputs?

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

Done. Good catch. Added a test.

+
+  _TransformNode(this.phase, this.transformer, this.primary);
+
+  /// Applies this transform. Outputs will be added to [updated]. Outputs that
+  /// were generated the last time this was applied but were not generated
+  /// this time will be added to [removed].

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

This signature is very confusing. The docstring implies that [updated] and
[removed] are effectively out parameters -- that they should come in empty and
will go out populated. But the code actually uses their input values. This
should be documented.

Modifying the parameters to an asynchronous function in general is confusing and
bug-prone. This should return a Future<Pair> or something similar.

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

Done.

+  Future apply(Map<AssetId, Asset> updated, Set<AssetId> removed) {
+    var transform = new _Transform(this);
+    return transformer.apply(transform).then((_) {
+      isDirty = false;
+
+      // Collect the outputs.
+      transform._outputs.forEach((id, asset) {
+        if (updated.containsKey(id)) {
+          // Report a collision.
+          phase.graph._resultsController.add(new ProcessResult(
+              new AssetCollisionException(id)));
+          // TODO(rnystrom): Define what happens after a collision occurs.

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

We should probably have some notion of a node and everything below it being in
an error state.

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

My current rough thoughts are that it would be left in a dirty state. That way
the next time an input is modified, it can try again. I'd like it to be
self-repairing like that. Think you're hacking on some Dart file while running
pub serve. You save in the middle of an edit with a syntax error and the dart2js
build fails in the background. The next time you save, it picks it up and tries
again.

But I'd like to do that in a separate patch just to keep this simpler/smaller
for now.

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

I agree; errors shouldn't bring everything down. My point was that we should
propagate the "error"-ness to any assets that were transitively generated by
this transform last time it was run.
For sure, I was just mentioning my thoughts on the question posed by the TODO.

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

Done.

+        } else {
+          updated[id] = asset;
+        }
+      });
+
+      // See which outputs are missing from the last run.
+      var outputIds = transform._outputs.keys.toSet();
+      var removedOutputs = outputs.difference(outputIds);
+      outputs = outputIds;
+      removed.addAll(removedOutputs);

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

We should be sure we test the case where one transform removes an output at the
same time that another one adds that output.

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

Done. It was doing the right thing, but it wasn't clear from the code. Added a
test and more docs.

+    });
+  }
+}
+
+/// A concrete implementation of [Transform].
+class _Transform implements Transform {

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

I really hate the pattern of an interface with a single, private implementation.
Just call this class Transform and export it.

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

It isn't that simple. It needs to have both a private constructor and other
private fields that are accessed here. That means if would stay in this file.
That interacts poorly with Dart's export semantics: if both barback.dart (by
exporting) and this define Transform, it will collide in files that import both
asset_graph and barback.

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

"lib/barback.dart" should only contain exports and maybe some top-level utility
functions. If there's a Barback class, it should go in "lib/src/barback.dart"
and be exported by "lib/barback.dart". Everything in "lib/src" should import
only from "lib/src"; every external library should import only from "lib".

I think that scheme gets around any export issues and allows you to put
Transform into its own library.

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

Yup, a later patch I'm working on does that.
Done.
Done.

+  final _TransformNode _node;
+
+  final _inputs = new Set<_AssetNode>();
+  final _outputs = new Map<AssetId, Asset>();
+
+  AssetId get primaryId => _node.primary.id;
+  Future<Asset> get primaryInput => getInput(primaryId);
+
+  _Transform(this._node);
+
+  Future<Asset> getInput(AssetId id) {
+    return new Future(() {
+      var node = _node.phase.inputs[id];
+      // TODO(rnystrom): Need to handle passthrough where an asset from a
+      // previous phase can be found.
+      if (node == null) {
+        _node.phase.graph._throwError(new MissingInputException(id));
+      }
+
+      // Keep track of which assets this transform depends on.
+      _inputs.add(node);
+      node.consumers.add(_node);
+      return node.asset;
+    });
+  }
+
+  void addOutput(AssetId id, Asset output) {
+    _outputs[id] = output;
+  }
+}