Add support for transformer clusters to barback.

Add support for transformer clusters to barback.

A cluster encapsulates a transformation graph in a form that can be used
similarly to a plain transformer.

R=rnystrom@google.com
BUG=

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

[Bob Nystrom, 2013-10-04 21:47:50]

I still don't feel like I have a solid big picture of how this hangs together,
but I like the general direction.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/asset_...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_node.dart:151: /// state of [node].
Would it be better now to say "for a node that is forwarded from [node]"?

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
File pkg/barback/lib/src/cluster_runner.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:15: /// A class that handles a single
transformer cluster.
"handles" -> "processes all of the phases in"

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:17: /// A cluster takes a many inputs,
processes them, and emits many outputs.
"a many" -> "many".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:35: // rid of this.
Add: "Once that's done, see if TransformerGroup and AssetCascade can be
unified."

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:43: var lastPhase = new Phase(cascade,
cluster.phases.first);
It feels weird to me that the phases within the cluster know the top level
cascade that contains them. Maybe this requires unifying cluster and cascade,
but I wish a phase only knew the immediate thing that owned it.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:70: for (var phase in _phases) {
Add comment, like "Process the first phase that needs to do work."

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:74: 
// If we get here, all phases are done processing.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/operat...
File pkg/barback/lib/src/operator.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/operat...
pkg/barback/lib/src/operator.dart:9: /// An interface that denotes classes that
can be used as Barback operators.
As per our discussion, this doesn't add much value. Let's just ditch it and make
the API that takes Transformer | Cluster just be untyped. Everywhere you use
"operator" just switch it back to "transformer".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:61: // TODO(nweiz): don't re-calculate this on
the fly all the time
"." and capitalize "Don't".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:66: /// [_inputs] or [_clusters]. Eeach input
asset has a PhaseInput in [_inputs].
"Eeach" -> "Each".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:110: Phase(this.cascade, Iterable<Operator>
operators)
We could keep this API typed if the transformers and clusters were split out
before calling this and were passed in as two separate collections. Do that?

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:133: var forwarder = new
PhaseForwarder(_clusters.length + 1);
Explain what the .length + 1 represents.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:193: }
Nit but add blank line after "}".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:273: }))..addAll(_clusters.values.map((input) {
Maybe it's just me, but using ".." here feels a bit gratuitous with multi-line
maps too. How about just:

var outputFutures = _inputs.values.map(...).toList();

outputFutures.addAll(_clusters.values.map(...));

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:18: /// assets are themselves marked
dirty. It should only be available if all
Can you clarify what "intermediate forwarded assets" means here?

Also, "It" -> "The forwarded asset", I think?

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:24: class PhaseForwarder {
"AssetForwarder"? I know it's for a Phase, but the thing it forwards is Assets.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:26: /// forwarded.
I think this should just be documented like a field, so: "The number of..."

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:28: /// Each cluster is a channel,
along with one channel for the [PhaseInput]
This makes me wonder if we could stratify things where Phase only contains
clusters and the "unclustered" loose transformers in a phase get put into their
own implicit cluster. Would that simplify things?

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:30: set channels(int value) {
"channels" -> "numChannels"

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:34: int _channels;
_numChannels

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/transf...
File pkg/barback/lib/src/transformer_cluster.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transformer_cluster.dart:22: /// following that containing
the cluster will be able to see its outputs,
"Phases following that" -> "Phases after the one".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transformer_cluster.dart:24: class TransformerCluster
implements Operator {
As per our discussion: "TransformerGroup".

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
File pkg/barback/test/package_graph/cluster_test.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:58: "app|foo.a": "foo",
Using "foo" both as the file name and content is a bit confusing here. Can you
use something different for the content?

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:143: ]), new
RewriteTransformer("x", "b")]
Nit but can you move "new Rewrite..." to its own line? Here, and below.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:177: test("parallel clusters
can't see one another's intermediate assets", () {
What happens if intermediates in two clusters collide? I'm guessing there should
be no collision.

Add a test.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:311: ]);
Style nit, but I think this is a better:

updateOperators("app", [[
  rewrite,
  new TransformerCluster([
    [new RewriteTransformer("a", "b")],
    [new RewriteTransformer("b", "c")]
  ])
]]);

[nweiz, 2013-10-07 23:21:30]

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/asset_...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_node.dart:151: /// state of [node].
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Would it be better now to say "for a node that is forwarded from [node]"?

No; there's a big difference between this and an [AssetForwarder], namely that
this doesn't continue to forward changes to the node.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
File pkg/barback/lib/src/cluster_runner.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:15: /// A class that handles a single
transformer cluster.
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "handles" -> "processes all of the phases in"

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:17: /// A cluster takes a many inputs,
processes them, and emits many outputs.
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "a many" -> "many".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:35: // rid of this.
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Add: "Once that's done, see if TransformerGroup and AssetCascade can be
> unified."

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:43: var lastPhase = new Phase(cascade,
cluster.phases.first);
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> It feels weird to me that the phases within the cluster know the top level
> cascade that contains them. Maybe this requires unifying cluster and cascade,
> but I wish a phase only knew the immediate thing that owned it.

I find the fact that we're passing around the cascade at all weird. It only does
two things: provides access to cross-package assets via the PackageGraph, and
provides a channel for reporting error information. I'd rather handle the former
through some intermediate interface, and the latter by piping errors through
AssetNodes.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:70: for (var phase in _phases) {
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Add comment, like "Process the first phase that needs to do work."

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:74: 
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> // If we get here, all phases are done processing.

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/operat...
File pkg/barback/lib/src/operator.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/operat...
pkg/barback/lib/src/operator.dart:9: /// An interface that denotes classes that
can be used as Barback operators.
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> As per our discussion, this doesn't add much value. Let's just ditch it and
make
> the API that takes Transformer | Cluster just be untyped. Everywhere you use
> "operator" just switch it back to "transformer".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:61: // TODO(nweiz): don't re-calculate this on
the fly all the time
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "." and capitalize "Don't".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:66: /// [_inputs] or [_clusters]. Eeach input
asset has a PhaseInput in [_inputs].
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "Eeach" -> "Each".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:110: Phase(this.cascade, Iterable<Operator>
operators)
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> We could keep this API typed if the transformers and clusters were split out
> before calling this and were passed in as two separate collections. Do that?

I think that would result in more code duplication -- we'd need to do that
splitting both places we call [new Phase].

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:133: var forwarder = new
PhaseForwarder(_clusters.length + 1);
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Explain what the .length + 1 represents.

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:193: }
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Nit but add blank line after "}".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase....
pkg/barback/lib/src/phase.dart:273: }))..addAll(_clusters.values.map((input) {
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Maybe it's just me, but using ".." here feels a bit gratuitous with multi-line
> maps too. How about just:
> 
> var outputFutures = _inputs.values.map(...).toList();
> 
> outputFutures.addAll(_clusters.values.map(...));

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:18: /// assets are themselves marked
dirty. It should only be available if all
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Can you clarify what "intermediate forwarded assets" means here?
> 
> Also, "It" -> "The forwarded asset", I think?

Rewritten this whole thing to hopefully make it clearer. I've added specific
terms for assets in different stages of the process, and updated the other
documentation/names to use those terms.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:24: class PhaseForwarder {
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "AssetForwarder"? I know it's for a Phase, but the thing it forwards is
Assets.

That's already a class that exists, and it's generally applicable to all assets
so  I think the name makes more sense for it.

The intention was this name was to follow the pattern of PhaseInput/PhaseOutput.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:26: /// forwarded.
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> I think this should just be documented like a field, so: "The number of..."

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:28: /// Each cluster is a channel,
along with one channel for the [PhaseInput]
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> This makes me wonder if we could stratify things where Phase only contains
> clusters and the "unclustered" loose transformers in a phase get put into
their
> own implicit cluster. Would that simplify things?

I considered that, but it produces an infinite recursion problem. Imagine you
have a phase with a single transformer. That transformer is represented as a
cluster, which contains a phase. What does that phase contain? It can't just
contain a transformer, it has to contain a cluster! It needs to bottom out at
some point, and the "raw" transformers provide a convenient way to do that.

We could add a class that exposes the same API as ClusterRunner but runs a set
of transformers directly. I didn't explore that route mostly because this
resulted in less code churn. It's a good avenue to explore for future refactors.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:30: set channels(int value) {
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "channels" -> "numChannels"

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:34: int _channels;
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> _numChannels

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/transf...
File pkg/barback/lib/src/transformer_cluster.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transformer_cluster.dart:22: /// following that containing
the cluster will be able to see its outputs,
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> "Phases following that" -> "Phases after the one".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transformer_cluster.dart:24: class TransformerCluster
implements Operator {
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> As per our discussion: "TransformerGroup".

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
File pkg/barback/test/package_graph/cluster_test.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:58: "app|foo.a": "foo",
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Using "foo" both as the file name and content is a bit confusing here. Can you
> use something different for the content?

Done. I did that because that's the default contents if none is specified, but I
can see how it would be confusing when it's explicitly spelled out.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:143: ]), new
RewriteTransformer("x", "b")]
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Nit but can you move "new Rewrite..." to its own line? Here, and below.

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:177: test("parallel clusters
can't see one another's intermediate assets", () {
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> What happens if intermediates in two clusters collide? I'm guessing there
should
> be no collision.
> 
> Add a test.

Done.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/test/package_g...
pkg/barback/test/package_graph/cluster_test.dart:311: ]);
On 2013/10/04 21:47:50, Bob Nystrom wrote:
> Style nit, but I think this is a better:
> 
> updateOperators("app", [[
>   rewrite,
>   new TransformerCluster([
>     [new RewriteTransformer("a", "b")],
>     [new RewriteTransformer("b", "c")]
>   ])
> ]]);

Done.

[Bob Nystrom, 2013-10-07 23:58:39]

LGTM.

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
File pkg/barback/lib/src/cluster_runner.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:43: var lastPhase = new Phase(cascade,
cluster.phases.first);
On 2013/10/07 23:21:31, nweiz wrote:
> On 2013/10/04 21:47:50, Bob Nystrom wrote:
> > It feels weird to me that the phases within the cluster know the top level
> > cascade that contains them. Maybe this requires unifying cluster and
cascade,
> > but I wish a phase only knew the immediate thing that owned it.
> 
> I find the fact that we're passing around the cascade at all weird. It only
does
> two things: provides access to cross-package assets via the PackageGraph, and
> provides a channel for reporting error information. I'd rather handle the
former
> through some intermediate interface, and the latter by piping errors through
> AssetNodes.

SGTM. Maybe add a TODO?

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/phase_...
pkg/barback/lib/src/phase_forwarder.dart:28: /// Each cluster is a channel,
along with one channel for the [PhaseInput]
On 2013/10/07 23:21:31, nweiz wrote:
> On 2013/10/04 21:47:50, Bob Nystrom wrote:
> > This makes me wonder if we could stratify things where Phase only contains
> > clusters and the "unclustered" loose transformers in a phase get put into
> their
> > own implicit cluster. Would that simplify things?
> 
> I considered that, but it produces an infinite recursion problem. Imagine you
> have a phase with a single transformer. That transformer is represented as a
> cluster, which contains a phase. What does that phase contain? It can't just
> contain a transformer, it has to contain a cluster! It needs to bottom out at
> some point, and the "raw" transformers provide a convenient way to do that.
> 
> We could add a class that exposes the same API as ClusterRunner but runs a set
> of transformers directly. I didn't explore that route mostly because this
> resulted in less code churn. It's a good avenue to explore for future
refactors.

Oh, right. :)

https://codereview.chromium.org/25376003/diff/15001/pkg/barback/lib/src/phase...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/25376003/diff/15001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase_forwarder.dart:31: /// are dirty, the final asset will
also be marked dirty.
This is really helpful.

[nweiz, 2013-10-08 00:30:39]

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
File pkg/barback/lib/src/cluster_runner.dart (right):

https://codereview.chromium.org/25376003/diff/5001/pkg/barback/lib/src/cluste...
pkg/barback/lib/src/cluster_runner.dart:43: var lastPhase = new Phase(cascade,
cluster.phases.first);
On 2013/10/07 23:58:39, Bob Nystrom wrote:
> On 2013/10/07 23:21:31, nweiz wrote:
> > On 2013/10/04 21:47:50, Bob Nystrom wrote:
> > > It feels weird to me that the phases within the cluster know the top level
> > > cascade that contains them. Maybe this requires unifying cluster and
> cascade,
> > > but I wish a phase only knew the immediate thing that owned it.
> > 
> > I find the fact that we're passing around the cascade at all weird. It only
> does
> > two things: provides access to cross-package assets via the PackageGraph,
and
> > provides a channel for reporting error information. I'd rather handle the
> former
> > through some intermediate interface, and the latter by piping errors through
> > AssetNodes.
> 
> SGTM. Maybe add a TODO?

Done, in Phase.

https://codereview.chromium.org/25376003/diff/15001/pkg/barback/lib/src/phase...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/25376003/diff/15001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase_forwarder.dart:31: /// are dirty, the final asset will
also be marked dirty.
On 2013/10/07 23:58:39, Bob Nystrom wrote:
> This is really helpful.

:)

[nweiz, 2013-10-08 00:31:00]

Committed patchset #5 manually as r28344 (presubmit successful).