Add the ability to dynamically modify transforms to barback.

Add the ability to dynamically modify transforms to barback.

R=rnystrom@google.com
BUG=

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

[Bob Nystrom, 2013-08-20 19:08:53]

Are you thinking pub serve will monitor pubspecs and load new transformers on
the fly?

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:88: //TODO(rnystrom): Better way of
specifying transformers and their ordering.
This is TODONE!

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/package_g...
pkg/barback/lib/src/package_graph.dart:120: _cascadeResults[package] = null;
What if the set of transformers are unchanged?

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:248: for (var output in
_outputs.values.map((queue) => queue.first)) {
The map here seems a bit gratuitous. How about just:

_next.addInput(queue.first);

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/transform_test.dart (right):

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:809: group("added/removed
transformers", () {
Let's move this to a separate test suite. This one's getting pretty large.

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:836: test("updateTransformers
doesn't re-run an old transformer", () {
What if the old transformer is updated to be in a different phase? Say you had:

[
  [rewrite "txt" -> "foo"],
  [rewriteInPlace "txt"]
]

And you update it to:

[
  [rewriteInPlace "txt"]
  [rewrite "txt" -> "foo"]
]

At that point, the transformers do need to be re-run since "txt"->"foo" now
needs to process the rewritten-in-place asset. Looking at the code, I think it
will do the right thing, but it would be good to test it.

Another one to consider is:


[
  [],
  [rewriteInPlace "txt"]
]

Updated to:

[
  [rewriteInPlace "txt"]
  [rewrite "txt" -> "foo"]
]

Here, rewrite "txt" -> "foo" hasn't changed phase, but it still needs to be
re-run.

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:958: return;
I'd like this to be more visible. How about pulling this test into a separate
file and then marking that as failing in the status file?

[nweiz, 2013-08-20 21:39:17]

I assume eventually pub serve will reload pubspecs on the fly, but that's not
the main reason for this patch. This is laying the groundwork for loading
transformers as plugins. We're going to need to have a "pub run"-style server
for doing so, and if we want to use that same server for "pub serve", we'll need
to be able to add the transformers we load to barback after they're loaded.

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/asset_cas...
pkg/barback/lib/src/asset_cascade.dart:88: //TODO(rnystrom): Better way of
specifying transformers and their ordering.
On 2013/08/20 19:08:54, Bob Nystrom wrote:
> This is TODONE!

Done.

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/package_g...
pkg/barback/lib/src/package_graph.dart:120: _cascadeResults[package] = null;
On 2013/08/20 19:08:54, Bob Nystrom wrote:
> What if the set of transformers are unchanged?

We always want to trigger a rebuild so that the caller can consistently expect
that [updateTransformers] will cause [results] to emit a result at some point in
the future.

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:248: for (var output in
_outputs.values.map((queue) => queue.first)) {
On 2013/08/20 19:08:54, Bob Nystrom wrote:
> The map here seems a bit gratuitous. How about just:
> 
> _next.addInput(queue.first);

Done.

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/transform_test.dart (right):

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:809: group("added/removed
transformers", () {
On 2013/08/20 19:08:54, Bob Nystrom wrote:
> Let's move this to a separate test suite. This one's getting pretty large.

Done.

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:836: test("updateTransformers
doesn't re-run an old transformer", () {
On 2013/08/20 19:08:54, Bob Nystrom wrote:
> What if the old transformer is updated to be in a different phase? Say you
had:
> 
> [
>   [rewrite "txt" -> "foo"],
>   [rewriteInPlace "txt"]
> ]
> 
> And you update it to:
> 
> [
>   [rewriteInPlace "txt"]
>   [rewrite "txt" -> "foo"]
> ]
> 
> At that point, the transformers do need to be re-run since "txt"->"foo" now
> needs to process the rewritten-in-place asset. Looking at the code, I think it
> will do the right thing, but it would be good to test it.

Barback sees that as two new transformers. A transformer is only considered to
be "the same" if it's in the same phase. I'll add a test, though.

> Another one to consider is:
> 
> 
> [
>   [],
>   [rewriteInPlace "txt"]
> ]
> 
> Updated to:
> 
> [
>   [rewriteInPlace "txt"]
>   [rewrite "txt" -> "foo"]
> ]
> 
> Here, rewrite "txt" -> "foo" hasn't changed phase, but it still needs to be
> re-run.

Added.

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:958: return;
On 2013/08/20 19:08:54, Bob Nystrom wrote:
> I'd like this to be more visible. How about pulling this test into a separate
> file and then marking that as failing in the status file?

It won't be around for more than a week or two; I've got in-progress patches
working towards fixing it. Given that, I'd rather leave this here for now.

[Bob Nystrom, 2013-08-20 22:18:12]

One request but LGTM either way.

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

https://codereview.chromium.org/23311006/diff/1/pkg/barback/lib/src/package_g...
pkg/barback/lib/src/package_graph.dart:120: _cascadeResults[package] = null;
On 2013/08/20 21:39:17, nweiz wrote:
> On 2013/08/20 19:08:54, Bob Nystrom wrote:
> > What if the set of transformers are unchanged?
> 
> We always want to trigger a rebuild so that the caller can consistently expect
> that [updateTransformers] will cause [results] to emit a result at some point
in
> the future.

Ensuring a result is sent is fine, but it sucks to discard the previously
generated assets and actually apply all of the transformers again from scratch.
Isn't there a way we can do the former without the latter?

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/transform_test.dart (right):

https://codereview.chromium.org/23311006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:958: return;
On 2013/08/20 21:39:17, nweiz wrote:
> On 2013/08/20 19:08:54, Bob Nystrom wrote:
> > I'd like this to be more visible. How about pulling this test into a
separate
> > file and then marking that as failing in the status file?
> 
> It won't be around for more than a week or two; I've got in-progress patches
> working towards fixing it. Given that, I'd rather leave this here for now.

SGTM.

[nweiz, 2013-08-20 22:31:48]

Committed patchset #2 manually as r26403 (presubmit successful).