Add events in barback to bubble up the dirty bit.

Add events in barback to bubble up the dirty bit.

This allows TransformNodes and Phases to notify their AssetCascade that they've
become dirty. This is important for transforms re-running their cascades when
their cross-package imports become dirty.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-08-01 18:10:33]

The implementation of this patch is solid, but I feel like the approach is
heavyweight. Using streams and stream pools seems like a lot of machinery just
to propagate some bits around.

I think streams are a good fit when an object wants to emit events without
knowing what it's emitting them to. But in this case, the objects emitting
onDirty events already know their listener:

Phase knows the AssetCascade that owns it.
TransformNode knows the Phase that owns it.

Streams also tend to imply asynchrony, which is definitely not the case here.

I like decoupling, but I think it's adding more complexity than it removes,
especially with having to remember to add and remove streams from the pools. How
about just having Phase call dirty() on its parent AssetCascade explicitly and
ditto for TransformNow->Phase?

Don't get me wrong, I think StreamPool is a useful class, I'm just not crazy
about using it here.

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

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:193: // kick off a build, even if that build does
nothing.
Why?

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/stream_po...
File pkg/barback/lib/src/stream_pool.dart (right):

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/stream_po...
pkg/barback/lib/src/stream_pool.dart:45: void addStream(Stream<T> stream) {
How about just "add()" (and "remove()" below)?

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/stream_po...
pkg/barback/lib/src/stream_pool.dart:62: }
_subscriptions.clear();

https://codereview.chromium.org/21446002/diff/1/pkg/barback/test/stream_pool_...
File pkg/barback/test/stream_pool_test.dart (right):

https://codereview.chromium.org/21446002/diff/1/pkg/barback/test/stream_pool_...
pkg/barback/test/stream_pool_test.dart:74: expect(newFuture(() =>
pool.stream.toList()),
Copy the comment from the similar test above here.

https://codereview.chromium.org/21446002/diff/1/pkg/barback/test/stream_pool_...
pkg/barback/test/stream_pool_test.dart:114: handleError: (error, sink) =>
sink.add(["error", error]))).toList();
Since you're just checking for emptiness, there's no real reason to make little
tuples here. Could just do sink.add(error).

[nweiz, 2013-08-01 20:30:52]

I really don't like having classes manually set fields on other classes, even
through methods like `dirty()`. Each class should be responsible for keeping
track of its own dirty state and determining how that relates to its children.
If the logic for how AssetCascade reacts to a Phase becoming dirty changes, that
should involve a code change in AssetCascade, not Phase.

I agree that streams are kind of clunky, but I think it makes the semantics
clearer and the code easier to understand on the whole. As an anecdote, when I
was reading this code for the first time, one of the most confusing aspects was
Phase calling [TransformNode.dirty]. Despite there being less code than the
current evented AssetNode, it was very difficult for me to parse the flow,
because the responsibilities of each class felt muddied.

This code is fundamentally complicated, and it's going to keep getting more
complicated for a while as we bring it closer to supporting all the use cases we
need it to. It's nice to keep the code complexity down, but it's critical to
keep the comprehension complexity down. That means we need clear lines between
the responsibilities of each class and clear means of communication with one
another. The more you can reason about a class's behavior by looking only at
that class, the better.

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

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:193: // kick off a build, even if that build does
nothing.
On 2013/08/01 18:10:33, Bob Nystrom wrote:
> Why?

API consistency. If a user calls [Barback.updateSources] or
[Barback.removeSources] then waits for the build to complete, that should always
terminate. If we don't even kick off a build when we can prove nothing needs
building, we run the risk of deadlocking users who do that.

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/stream_po...
File pkg/barback/lib/src/stream_pool.dart (right):

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/stream_po...
pkg/barback/lib/src/stream_pool.dart:45: void addStream(Stream<T> stream) {
On 2013/08/01 18:10:33, Bob Nystrom wrote:
> How about just "add()" (and "remove()" below)?

Done.

https://codereview.chromium.org/21446002/diff/1/pkg/barback/lib/src/stream_po...
pkg/barback/lib/src/stream_pool.dart:62: }
On 2013/08/01 18:10:33, Bob Nystrom wrote:
> _subscriptions.clear();

Done.

https://codereview.chromium.org/21446002/diff/1/pkg/barback/test/stream_pool_...
File pkg/barback/test/stream_pool_test.dart (right):

https://codereview.chromium.org/21446002/diff/1/pkg/barback/test/stream_pool_...
pkg/barback/test/stream_pool_test.dart:74: expect(newFuture(() =>
pool.stream.toList()),
On 2013/08/01 18:10:33, Bob Nystrom wrote:
> Copy the comment from the similar test above here.

Done.

https://codereview.chromium.org/21446002/diff/1/pkg/barback/test/stream_pool_...
pkg/barback/test/stream_pool_test.dart:114: handleError: (error, sink) =>
sink.add(["error", error]))).toList();
On 2013/08/01 18:10:33, Bob Nystrom wrote:
> Since you're just checking for emptiness, there's no real reason to make
little
> tuples here. Could just do sink.add(error).

Done.

[Bob Nystrom, 2013-08-02 16:33:43]

LGTM.

Streams still feel heavyweight and error-prone to me (we have to always remember
to subscribe and unsubcribe when adding and removing TransformNodes from their
Phase), but maybe it will be a better fit as the codebase grows.

[nweiz, 2013-08-05 20:23:14]

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