Add support for lazy transformers.

Add support for lazy transformers.

An upcoming CL will support propagating laziness through DryRunTransformers.

R=rnystrom@google.com
BUG=16077
Committed: https://code.google.com/p/dart/source/detail?r=32516

[Bob Nystrom, 2014-01-30 19:33:44]

When you get some time, let's talk about the names for "materialize" and "dry
run".

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:56: /// This is only non-null for lazy asset
nodes (see
"only non-null for lazy" -> "null for non-lazy"

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:59: Function _lazyCallback;
I'm all for using callbacks in barback, but isn't this inconsistent with the
rest of barback which uses streams? Shouldn't this be a onMaterialize stream?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:182: /// For the most party, this node works
like any other dirty node. However,
I'm always trying to maximize my party too, but here it should just be "for the
most part".

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:184: /// available as soon as possible like
they would for a non-lazy node.
they -> it

TransformNodes are gender-less, not gender-neutral. :)

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:247: /// Lazy nodes aren't expected to have
their values generated until it's
"it's necessary" -> "needed".

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/dry_run_...
File pkg/barback/lib/src/dry_run_transform.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/dry_run_...
pkg/barback/lib/src/dry_run_transform.dart:15: class DryRunTransform extends
BaseTransform {
Do we want to allow dry run transformers to read concrete inputs?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/dry_run_...
pkg/barback/lib/src/dry_run_transform.dart:25: /// [Transformer.apply] should
emit the corresponding asset as well.
"should" -> "must".

What happens if it emits an asset that was *not* previously emitted by dry run?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
File pkg/barback/lib/src/lazy_transformer.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
pkg/barback/lib/src/lazy_transformer.dart:15: abstract class LazyTransformer
extends DryRunTransformer {}
Is this so that you can distinguish between:

1. A transformer that can declare its outputs but also wants to be run eagerly.
2. A transformer that declares its outputs but only generates them lazily.

?

I like being able to express this distinction, but I think using types for this
may be confusing to users. They may not realize that choosing which of two
identical interfaces to implement affects the runtime behavior of their
transformer.

It's also less flexible, since this is a static declaration. A transformer can't
choose to be lazy sometimes and eager others.

How about just having an isLazy getter on DryRunTransformer?

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

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/phase.da...
pkg/barback/lib/src/phase.dart:139: node.materialize();
Why is this being materialized?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/phase.da...
pkg/barback/lib/src/phase.dart:187: return _outputs[id].output..materialize();
Nit, but I'm really not a fan of using a single cascade just to get back to the
original receiver.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
File pkg/barback/lib/src/serialize.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:36: /// Converts [callback] into a [SendPort]
which which another isolate can call
I think you had something similar in the previous lazy patch. In both cases, the
behavior of these is really confusing and could use more documentation.
Something like:

"Returns a new SendPort. When a second SendPort is sent through that one, the
callback will be invoked and its result sent to *that* SendPort."

Or something along those lines. This does make we wonder if there's a simpler
way to do this?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:49: /// to the value of [Future].
Ditto above comment.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:55: future.catchError(null);
I don't understand how passing a null catch handler accomplishes this. I'd
expect this to be a misuse of the Future API. What am I missing here?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:62: /// A helper method for sending the
result of [future] across [sendPort].
"helper method" doesn't add much. How about "Sends the result of [future] to
[sendPort] when it completes."

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:102: return () {
What's the outer function wrapper for?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform.dart:23: Transform(TransformNode node,
this._outputs, LogFunction logFunction)
Transform is in barback's public API, but TransformNode is not, and shouldn't
be. I don't think this constructor should be publicly visible because of that.
(That's why this code previously had the little createTransform() function.

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

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/utils.da...
pkg/barback/lib/src/utils.dart:145: Map listToMap(Iterable iter, keyFn(element),
valueFn(element)) {
Is this any different from new Map.fromIterable()?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/utils.da...
pkg/barback/lib/src/utils.dart:170: Map mapMapKeys(Map map, fn(key, value)) =>
mapMap(map, fn, (_, value) => value);
Not in this patch, but we should look at moving these into quiver.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:86: // properly.
File a bug for this.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:140: test("once a lazy
transformer is materialized, it runs eagerly afterwards",
I know this is what we discussed, but I wonder if this is what the user wants.
Maybe lazy transformers should stay lazy?

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:147: 
Comment: // Request the asset once to force it to be materialized.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/lazy_bad.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/lazy_bad.dart:14: /// A lazy transformer that throw
an exception after [dryRun].
throw -> throws

[nweiz, 2014-01-31 03:43:27]

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:56: /// This is only non-null for lazy asset
nodes (see
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> "only non-null for lazy" -> "null for non-lazy"

Done.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:59: Function _lazyCallback;
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> I'm all for using callbacks in barback, but isn't this inconsistent with the
> rest of barback which uses streams? Shouldn't this be a onMaterialize stream?

I chose a callback instead because it's guaranteed to only fire once, and the
person who instantiates a lazy node or makes a node lazy should always register
a callback for it.

I'd make this a Future if that worked with synchronous callbacks and I could
somehow return a Future from [AssetNodeController.lazy].

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:182: /// For the most party, this node works
like any other dirty node. However,
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> I'm always trying to maximize my party too, but here it should just be "for
the
> most part".

Done.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:184: /// available as soon as possible like
they would for a non-lazy node.
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> they -> it
> 
> TransformNodes are gender-less, not gender-neutral. :)

"They" actually refers to "the owner" here, who is gender-neutral.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/asset_no...
pkg/barback/lib/src/asset_node.dart:247: /// Lazy nodes aren't expected to have
their values generated until it's
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> "it's necessary" -> "needed".

Done.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/dry_run_...
File pkg/barback/lib/src/dry_run_transform.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/dry_run_...
pkg/barback/lib/src/dry_run_transform.dart:15: class DryRunTransform extends
BaseTransform {
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Do we want to allow dry run transformers to read concrete inputs?

I believe we do. We can optimize for the case where they don't actually consume
the contents of the inputs, but I think we should let them read the contents if
they want to.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/dry_run_...
pkg/barback/lib/src/dry_run_transform.dart:25: /// [Transformer.apply] should
emit the corresponding asset as well.
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> "should" -> "must".
> 
> What happens if it emits an asset that was *not* previously emitted by dry
run?

I don't think we should say "must", since we do handle this case pretty
gracefully. If they emit an undeclared output, that works fine. The only
behavioral degradation is that if the transform is lazy, accessing the
undeclared output won't trigger materialization (since we don't know that it
exists yet).

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
File pkg/barback/lib/src/lazy_transformer.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
pkg/barback/lib/src/lazy_transformer.dart:15: abstract class LazyTransformer
extends DryRunTransformer {}
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Is this so that you can distinguish between:
> 
> 1. A transformer that can declare its outputs but also wants to be run
eagerly.
> 2. A transformer that declares its outputs but only generates them lazily.
> 
> ?

Yes. The former is broadly useful (at least in theory), and we should encourage
any transformer that can to implement it.

> I like being able to express this distinction, but I think using types for
this
> may be confusing to users. They may not realize that choosing which of two
> identical interfaces to implement affects the runtime behavior of their
> transformer.
> 
> It's also less flexible, since this is a static declaration. A transformer
can't
> choose to be lazy sometimes and eager others.
> 
> How about just having an isLazy getter on DryRunTransformer?

A getter seems to invite the question "when is the getter accessed?". It's not
clear to me how the class communicates with a getter when laziness should be
activated or deactivated, or indeed what activating and deactivating laziness
means. If I toggle [isLazy] to false, does that mean all my existing lazy assets
become materialized?

I like having the conceptual clarity between being always and never lazy. I
think having laziness declared in the type of the class also helps surface to
users reading the docs that this transformer works differently than others.

I get what you're saying about checking interfaces being confusing, but I think
that's a problem to be solved with documentation. I think the fact that
LazyTransformer doesn't add any additional members is actually helpful here; it
prompts users to ask the question "so why does it exist?", thus leading them to
the documentation which will explain its purpose.

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

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/phase.da...
pkg/barback/lib/src/phase.dart:139: node.materialize();
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Why is this being materialized?

The phase needs its inputs to be available if it's going to process them. A
future CL will make it so that lazy assets are piped through dry run
transformers correctly, which will make this materialization happen closer to
where it's specifically needed.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/phase.da...
pkg/barback/lib/src/phase.dart:187: return _outputs[id].output..materialize();
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Nit, but I'm really not a fan of using a single cascade just to get back to
the
> original receiver.

I think it's nicer than making a variable, but okay.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
File pkg/barback/lib/src/serialize.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:36: /// Converts [callback] into a [SendPort]
which which another isolate can call
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> I think you had something similar in the previous lazy patch. In both cases,
the
> behavior of these is really confusing and could use more documentation.
> Something like:
> 
> "Returns a new SendPort. When a second SendPort is sent through that one, the
> callback will be invoked and its result sent to *that* SendPort."

I don't like how this description exposes the implementation details of these
functions. From the user's perspective, [serializeCallback] returns a black box
that can be passed between isolates and deserialized to a callback via
[deserializeCallback].

> Or something along those lines. This does make we wonder if there's a simpler
> way to do this?

These are actually no longer used, now that LazyAsset doesn't exist anymore.
I'll stash them for now.

I'm pretty sure this is the least complicated way to do this sort of thing,
though. The only way to send a push notification across isolates is by sending a
SendPort. Due to the lack of any sort of auto-serialization, this is as close as
we can come to wrapping this logic in a nicer API.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:49: /// to the value of [Future].
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Ditto above comment.

N/A

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:55: future.catchError(null);
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> I don't understand how passing a null catch handler accomplishes this. I'd
> expect this to be a misuse of the Future API. What am I missing here?

"null" means approximately the same thing as "() {}" to dart:async. They pass
null to futures all over the place in their code, so I assumed it was kosher
externally, too.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:62: /// A helper method for sending the
result of [future] across [sendPort].
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> "helper method" doesn't add much. How about "Sends the result of [future] to
> [sendPort] when it completes."

N/A

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform.dart:23: Transform(TransformNode node,
this._outputs, LogFunction logFunction)
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Transform is in barback's public API, but TransformNode is not, and shouldn't
> be. I don't think this constructor should be publicly visible because of that.
> (That's why this code previously had the little createTransform() function.

In general I prefer exposing constructors that aren't meant to be invoked to
doing weird private-but-wrapped-in-a-non-exposed-function shenanigans. See for
example the Task constructor in scheduled_test.

In this case in particular, [BaseTransform] already has to expose its
constructor so its subclasses can call it, so it seemed very weird to have that
exposed without this one exposed.

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

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/utils.da...
pkg/barback/lib/src/utils.dart:145: Map listToMap(Iterable iter, keyFn(element),
valueFn(element)) {
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Is this any different from new Map.fromIterable()?

It's not. Removed.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/utils.da...
pkg/barback/lib/src/utils.dart:170: Map mapMapKeys(Map map, fn(key, value)) =>
mapMap(map, fn, (_, value) => value);
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Not in this patch, but we should look at moving these into quiver.

Absolutely.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:86: // properly.
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> File a bug for this.

Done.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:140: test("once a lazy
transformer is materialized, it runs eagerly afterwards",
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> I know this is what we discussed, but I wonder if this is what the user wants.
> Maybe lazy transformers should stay lazy?

I think this is correct. I think when the user is working on their app, getting
a second or two's jump on dart2js can be handy.

If this turns out to be wrong, the way the code is structured means it won't be
too hard to switch to always-lazy.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:147: 
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Comment: // Request the asset once to force it to be materialized.

Done.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/transformer...
File pkg/barback/test/transformer/lazy_bad.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/transformer...
pkg/barback/test/transformer/lazy_bad.dart:14: /// A lazy transformer that throw
an exception after [dryRun].
On 2014/01/30 19:33:44, Bob Nystrom wrote:
> throw -> throws

Done.

[Bob Nystrom, 2014-01-31 18:28:53]

The code looks good. All I want to do before this lands is discuss naming for
"dry run" / "lazy" / "materialize".

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
File pkg/barback/lib/src/lazy_transformer.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
pkg/barback/lib/src/lazy_transformer.dart:15: abstract class LazyTransformer
extends DryRunTransformer {}
On 2014/01/31 03:43:27, nweiz wrote:
> On 2014/01/30 19:33:44, Bob Nystrom wrote:
> Yes. The former is broadly useful (at least in theory), and we should
encourage
> any transformer that can to implement it.

+1.

> A getter seems to invite the question "when is the getter accessed?". It's not
> clear to me how the class communicates with a getter when laziness should be
> activated or deactivated, or indeed what activating and deactivating laziness
> means. If I toggle [isLazy] to false, does that mean all my existing lazy
assets
> become materialized?
> 
> I like having the conceptual clarity between being always and never lazy. I
> think having laziness declared in the type of the class also helps surface to
> users reading the docs that this transformer works differently than others.
> 
> I get what you're saying about checking interfaces being confusing, but I
think
> that's a problem to be solved with documentation. I think the fact that
> LazyTransformer doesn't add any additional members is actually helpful here;
it
> prompts users to ask the question "so why does it exist?", thus leading them
to
> the documentation which will explain its purpose.

How about we make it a method on DryRunTransform? So while the transformer is
doing the dry run, one thing it can do is tell barback, "hey, you should run me
lazily." That gives it control at runtime over this, and is the natural point in
time (while its looking at its inputs) where it should be expressed.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
File pkg/barback/lib/src/serialize.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:36: /// Converts [callback] into a [SendPort]
which which another isolate can call
On 2014/01/31 03:43:27, nweiz wrote:
> On 2014/01/30 19:33:44, Bob Nystrom wrote:
> > I think you had something similar in the previous lazy patch. In both cases,
> the
> > behavior of these is really confusing and could use more documentation.
> > Something like:
> > 
> > "Returns a new SendPort. When a second SendPort is sent through that one,
the
> > callback will be invoked and its result sent to *that* SendPort."
> 
> I don't like how this description exposes the implementation details of these
> functions. From the user's perspective, [serializeCallback] returns a black
box
> that can be passed between isolates and deserialized to a callback via
> [deserializeCallback].

I'm OK with that too, but in that case, I wouldn't annotate the return type as
SendPort. That makes the opacity explicit.

> These are actually no longer used, now that LazyAsset doesn't exist anymore.
> I'll stash them for now.

SGTM.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/serializ...
pkg/barback/lib/src/serialize.dart:55: future.catchError(null);
On 2014/01/31 03:43:27, nweiz wrote:
> On 2014/01/30 19:33:44, Bob Nystrom wrote:
> > I don't understand how passing a null catch handler accomplishes this. I'd
> > expect this to be a misuse of the Future API. What am I missing here?
> 
> "null" means approximately the same thing as "() {}" to dart:async. They pass
> null to futures all over the place in their code, so I assumed it was kosher
> externally, too.

Oh, wow. That's totally news to me. They certainly don't document that. I would
expect adding null to mean "no handler", which would then top level an error.

I'd more comfortable if we used an explicit no-op fn here.

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/test/package_gra...
pkg/barback/test/package_graph/lazy_transformer_test.dart:140: test("once a lazy
transformer is materialized, it runs eagerly afterwards",
On 2014/01/31 03:43:27, nweiz wrote:
> On 2014/01/30 19:33:44, Bob Nystrom wrote:
> > I know this is what we discussed, but I wonder if this is what the user
wants.
> > Maybe lazy transformers should stay lazy?
> 
> I think this is correct. I think when the user is working on their app,
getting
> a second or two's jump on dart2js can be handy.
> 
> If this turns out to be wrong, the way the code is structured means it won't
be
> too hard to switch to always-lazy.

SGTM!

[nweiz, 2014-02-04 01:43:25]

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
File pkg/barback/lib/src/lazy_transformer.dart (right):

https://codereview.chromium.org/149243009/diff/1/pkg/barback/lib/src/lazy_tra...
pkg/barback/lib/src/lazy_transformer.dart:15: abstract class LazyTransformer
extends DryRunTransformer {}
On 2014/01/31 18:28:53, Bob Nystrom wrote:
> How about we make it a method on DryRunTransform? So while the transformer is
> doing the dry run, one thing it can do is tell barback, "hey, you should run
me
> lazily." That gives it control at runtime over this, and is the natural point
in
> time (while its looking at its inputs) where it should be expressed.

I think the conceptual complexity of a transformer that's sometimes lazy is a
lot higher than one that always is, and I don't think we have a clear use-case
for it anyway. If someone really wanted to do so, they could accomplish it with
a transformer group, but I don't think they're likely to.

[Bob Nystrom, 2014-02-05 21:12:09]

What do you think of:

materialize()                   -> apply()
materializeAllTransforms()      -> applyAllTransforms()
DryRunTransform                 -> TransformDeclaration
DryRunTransform.addOutputId()   -> .declareOutput()
DryRunTransformer               -> DeclaredTransformer
DryRunTransformer.dryRun()      -> .declareOutputs()
LazyTransformer                 -> LazyTransformer
TransformNode._isLazy           -> _isDeferred
TransformNode._applyImmediate() -> _apply()
TransformNode._applyLazy()      -> _declare()

[nweiz, 2014-02-05 21:43:54]

On 2014/02/05 21:12:09, Bob Nystrom wrote:
> What do you think of:
> 
> materialize()                   -> apply()
> materializeAllTransforms()      -> applyAllTransforms()

This is confusing, since we already use "apply" to mean something different in
Barback. I'd prefer either "force" or "materialize".

> DryRunTransform                 -> TransformDeclaration

I really think this should still end in "Transform", since the API is very
similar to the base Transform API. Also, "TransformDeclaration" sounds like it's
a declaration (which it's not) and like it's declaring a Transform (which it's
not).

> DryRunTransform.addOutputId()   -> .declareOutput()

SGTM

> DryRunTransformer               -> DeclaredTransformer

This sounds like the transformer has been declared, which is inaccurate. We're
trying to communicate that the transformer *can* declare things.

> DryRunTransformer.dryRun()      -> .declareOutputs()
> LazyTransformer                 -> LazyTransformer

SGTM

> TransformNode._isLazy           -> _isDeferred

I don't think it's worth adding a new, different word here.

> TransformNode._applyImmediate() -> _apply()
> TransformNode._applyLazy()      -> _declare()

I'm open to changing these, but I don't like [_declare]. [_applyLazy]
specifically only runs when lazy asset nodes are going to be emitted, which is
different than running [declareOutputs] on a non-lazy transformer to generate
dirty but non-lazy asset nodes.

[Bob Nystrom, 2014-02-05 21:51:40]

On 2014/02/05 21:43:54, nweiz wrote:
> On 2014/02/05 21:12:09, Bob Nystrom wrote:
> > What do you think of:
> > 
> > materialize()                   -> apply()
> > materializeAllTransforms()      -> applyAllTransforms()
> 
> This is confusing, since we already use "apply" to mean something different in
> Barback. I'd prefer either "force" or "materialize".

Does "apply" mean something noticeably different here? That function basically
means "fire up the transformers", right? That's pretty much "apply". Would
something like "beginApplies" work better?

I'm OK with "force", but I worry a bit that it will come across as sounding
synchronous or immediate.

> > DryRunTransform                 -> TransformDeclaration
> 
> I really think this should still end in "Transform", since the API is very
> similar to the base Transform API. Also, "TransformDeclaration" sounds like
it's
> a declaration (which it's not) and like it's declaring a Transform (which it's
> not).

DeclaringTransform?

> 
> > DryRunTransform.addOutputId()   -> .declareOutput()
> 
> SGTM
> 
> > DryRunTransformer               -> DeclaredTransformer
> 
> This sounds like the transformer has been declared, which is inaccurate. We're
> trying to communicate that the transformer *can* declare things.

DeclaringTransformer?

> > DryRunTransformer.dryRun()      -> .declareOutputs()
> > LazyTransformer                 -> LazyTransformer
> 
> SGTM
> 
> > TransformNode._isLazy           -> _isDeferred
> 
> I don't think it's worth adding a new, different word here.

My thought here was that it distinguishes "lazy", which describes a permanent
feature of some kinds of transformers from "deferred" which describes the
transient state of a lazy transformer which has not currently been applied. When
reading the code, I found it surprising that "lazy" could change. But if you
really like it, I'm OK with it too.

> 
> > TransformNode._applyImmediate() -> _apply()
> > TransformNode._applyLazy()      -> _declare()
> 
> I'm open to changing these, but I don't like [_declare]. [_applyLazy]
> specifically only runs when lazy asset nodes are going to be emitted, which is
> different than running [declareOutputs] on a non-lazy transformer to generate
> dirty but non-lazy asset nodes.

I'm not crazy about my suggestions either. I just found the current names
confusing and/or backwards. I'd expect "applyLazy" to mean "run apply() on the
lazy transformers" when it's actually the opposite of that. Likewise, I expected
"applyImmediate" to mean "apply everything immediately, even lazy stuff".

Maybe:

_applyImmediate -> _applyNonlazy()
_applyLazy -> _declareLazy()

?

[nweiz, 2014-02-05 22:02:03]

On 2014/02/05 21:51:40, Bob Nystrom wrote:
> Does "apply" mean something noticeably different here? That function basically
> means "fire up the transformers", right? That's pretty much "apply". Would
> something like "beginApplies" work better?

Elsewhere, "apply" means "start running your transformer(s)". "Materialize"
means "be willing to run your transformer(s) next time [apply] is called".


> DeclaringTransform?
> DeclaringTransformer?

I don't love these, but I don't hate them.

> My thought here was that it distinguishes "lazy", which describes a permanent
> feature of some kinds of transformers from "deferred" which describes the
> transient state of a lazy transformer which has not currently been applied.
When
> reading the code, I found it surprising that "lazy" could change. But if you
> really like it, I'm OK with it too.

Yeah, I understand the distinction, but I think using new terminology to draw
attention to it adds more confusion than it removes.

> I'm not crazy about my suggestions either. I just found the current names
> confusing and/or backwards. I'd expect "applyLazy" to mean "run apply() on the
> lazy transformers" when it's actually the opposite of that. Likewise, I
expected
> "applyImmediate" to mean "apply everything immediately, even lazy stuff".

"applyImmediate" does mean "apply everything immediately, even lazy stuff". It's
what you call when the transform is no longer in lazy mode. I see what you mean
about "applyLazy", though.

> Maybe:
> 
> _applyImmediate -> _applyNonlazy()
> _applyLazy -> _declareLazy()
> 
> ?

I like "_declareLazy", but I'm not a fan of "_applyNonLazy" because it implies
that it won't be run on a LazyTransformer, which is inaccurate.

[Bob Nystrom, 2014-02-05 23:00:45]

On 2014/02/05 22:02:03, nweiz wrote:
> On 2014/02/05 21:51:40, Bob Nystrom wrote:
> > Does "apply" mean something noticeably different here? That function
basically
> > means "fire up the transformers", right? That's pretty much "apply". Would
> > something like "beginApplies" work better?
> 
> Elsewhere, "apply" means "start running your transformer(s)". "Materialize"
> means "be willing to run your transformer(s) next time [apply] is called".

Ah, OK. I think "force" is better then.

> > My thought here was that it distinguishes "lazy", which describes a
permanent
> > feature of some kinds of transformers from "deferred" which describes the
> > transient state of a lazy transformer which has not currently been applied.
> When
> > reading the code, I found it surprising that "lazy" could change. But if you
> > really like it, I'm OK with it too.
> 
> Yeah, I understand the distinction, but I think using new terminology to draw
> attention to it adds more confusion than it removes.

Okey dokey.

> 
> > I'm not crazy about my suggestions either. I just found the current names
> > confusing and/or backwards. I'd expect "applyLazy" to mean "run apply() on
the
> > lazy transformers" when it's actually the opposite of that. Likewise, I
> expected
> > "applyImmediate" to mean "apply everything immediately, even lazy stuff".
> 
> "applyImmediate" does mean "apply everything immediately, even lazy stuff".
It's
> what you call when the transform is no longer in lazy mode. I see what you
mean
> about "applyLazy", though.
> 
> > Maybe:
> > 
> > _applyImmediate -> _applyNonlazy()
> > _applyLazy -> _declareLazy()
> > 
> > ?
> 
> I like "_declareLazy", but I'm not a fan of "_applyNonLazy" because it implies
> that it won't be run on a LazyTransformer, which is inaccurate.

How about _declareLazy() and _applyAll()?

- bob

[Bob Nystrom, 2014-02-07 21:02:27]

LGTM. This is really nice!

Can you file two bugs:

1. Document this.
2. Make the dart2js transformer in pub declaring and lazy.

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/lib/src/tra...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/lib/src/tra...
pkg/barback/lib/src/transform_node.dart:239: /// Applies the transform in
declarative mode so that it produces lazy outputs.
Long line.

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/test/packag...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/test/packag...
pkg/barback/test/package_graph/lazy_transformer_test.dart:101: // test("a lazy
asset piped into a declaring transformer is compiled on-demand",
Long line.

[nweiz, 2014-02-10 20:14:04]

> 1. Document this.

https://code.google.com/p/dart/issues/detail?id=16684

> 2. Make the dart2js transformer in pub declaring and lazy.

https://code.google.com/p/dart/issues/detail?id=16076

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/lib/src/tra...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/lib/src/tra...
pkg/barback/lib/src/transform_node.dart:239: /// Applies the transform in
declarative mode so that it produces lazy outputs.
On 2014/02/07 21:02:28, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/test/packag...
File pkg/barback/test/package_graph/lazy_transformer_test.dart (right):

https://codereview.chromium.org/149243009/diff/110001/pkg/barback/test/packag...
pkg/barback/test/package_graph/lazy_transformer_test.dart:101: // test("a lazy
asset piped into a declaring transformer is compiled on-demand",
On 2014/02/07 21:02:28, Bob Nystrom wrote:
> Long line.

Done.

[nweiz, 2014-02-10 20:18:12]

Committed patchset #4 manually as r32516 (presubmit successful).