Issue 16854005: First pass at build dependency graph for barback.

Bob Nystrom

Here's my first stab at the asset graph core. Give me your feedbacks.

7 years, 6 months ago (2013-06-12 22:50:06 UTC) #1

nweiz

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#newcode10 pkg/barback/lib/barback.dart:10: // TODO(rnystrom): Is this the prefix we want to ...

7 years, 6 months ago (2013-06-14 00:57:57 UTC) #2

Bob Nystrom

Should be better now. Thanks, as always, for the thorough review. The code and tests ...

7 years, 6 months ago (2013-06-17 23:35:05 UTC) #3

Should be better now. Thanks, as always, for the thorough review. The code and
tests are much better than they would be without it.

I still want to beef up the tests some more over time, but it's hard for me to
get a clear picture of which cases need more testing when right now it's just a
pure API with nothing concrete using it yet.

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:10: // TODO(rnystrom): Is this the prefix we want
to use?
On 2013/06/14 00:57:57, nweiz wrote:
> yes

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:14: class AssetId {
On 2013/06/14 00:57:57, nweiz wrote:
> I'd rather see each of these classes in its own library that gets exported
here.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:15: /// Parses an [AssetId] string of the form
"package|path/to/asset.txt".
On 2013/06/14 00:57:57, nweiz wrote:
> Why "|"? Why not ":"?

I did ":" at first, but I worried that it makes asset IDs look like URLs with
random schemes, which they are definitely not. This makes it clearer that they
are different, and gives them a distinct look. At a glance you can see it's a
package ID and not a path or URL.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:16: static AssetId parse(String description) {
On 2013/06/14 00:57:57, nweiz wrote:
> Why isn't this a constructor? I thought we decided that it didn't make sense
to
> separate out "parse" methods as statics.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:17: var parts = description.split("|");
On 2013/06/14 00:57:57, nweiz wrote:
> Assert that there's only two parts here.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:27: : path = pathos.normalize(path);
On 2013/06/14 00:57:57, nweiz wrote:
> This will break on Windows, since the asset paths are presumably all going to
be
> POSIX-style. We should store a POSIX-style builder and use that instead.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:34: if (other is! AssetId) return false;
On 2013/06/14 00:57:57, nweiz wrote:
> Style nit: fold this into the returned expression.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:42: return new AssetId(package,
"$path.$extension");
On 2013/06/14 00:57:57, nweiz wrote:
> Style nit: =>

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:46: var newPath = path.substring(0, path.length -
extension.length) +
On 2013/06/14 00:57:57, nweiz wrote:
> pathos.withoutExtension

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:54: }
On 2013/06/14 00:57:57, nweiz wrote:
> All these members will need documentation.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:56: /// An identifiable blob of data. Assets may
come from the file system, or
On 2013/06/14 00:57:57, nweiz wrote:
> Clarify what "identifiable" means.
> 
> Also, move the second sentence into its own paragraph. Same goes for several
> other doc comments.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:59: Asset();
On 2013/06/14 00:57:57, nweiz wrote:
> Why does this class have a constructor? It seems like it would work fine as an
> interface; there's no behavior we get from extending it.

Because it has other named constructors, it doesn't get an automatic unnamed
constructor. That means the subclasses _FileAsset and _StringAsset don't have a
superclass constructor to invoke if this is omitted.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:73: factory Asset.deserialize(data) {
On 2013/06/14 00:57:57, nweiz wrote:
> The rest of this class makes me think that Asset can be implemented outside of
> the barback package,

That is a use case I had in mind.

> but this constructor will break if users do that.

Good point.

> Maybe the thing to do is to use the serialization library so we can
legitimately
> support external subclasses of this class.

I'll add a TODO. This is here for the plug-in stuff which is still being
implemented. Once that becomes "real" code, using serialization is probably the
right thing to do.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:81: String readAsString();
On 2013/06/14 00:57:57, nweiz wrote:
> This method seems weird to me. Why do the assets need to implement it if they
> also implement [read]?

Convenience.

> Why is it synchronous when [read] is asynchronous?

Convenience.

> What does it do with binary-only assets?

Good question. I've got some half-formed ideas here, but nothing fleshed out
yet. I'll add a TODO.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:89: final File _file;
On 2013/06/14 00:57:57, nweiz wrote:
> I don't like storing a [File] object. I like the pub style of treating the
> [File] APIs as weird-looking static methods.

File is just a wrapper around a path. If I don't store it, each method will just
create one every time and throw it away. Seemed cleaner this way.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:101: class _StringAsset extends Asset {
On 2013/06/14 00:57:57, nweiz wrote:
> What about in-memory binary assets? It seems like it might be better to just
> have an in-memory asset class that stores the asset as a byte array, and have
a
> convenience method on Asset that gets the string version if the user knows
it's
> text.

Yeah, I don't have any real support for binary assets yet. It may be worth
having StringAsset for text assets because they may be faster/more compact in
the VM. But having something for in-memory binary assets will be good too. Added
a TODO.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:107: // TODO(rnystrom): Handle encoding?
On 2013/06/14 00:57:57, nweiz wrote:
> Absolutely!

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:108: Stream<List<int>> read() => new
Stream<List<int>>.fromIterable([_contents.codeUnits]);
On 2013/06/14 00:57:57, nweiz wrote:
> This is broken for a lot of non-ASCII text. [codeUnits] will return numbers >=
> 256, which will choke anything that expects a byte stream.
> 
> Also, long line.

This code path isn't being used or tested yet anyway, so just changed it to
throw UnimplementedError for now and added a TODO. I'll make it do something
less done when I actually implement. :)

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:133: /// API for locating and accessing packages on
disc. Implemented by pub and
On 2013/06/14 00:57:57, nweiz wrote:
> "disc" -> "disk"

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:136: /// The names of all packages that can be
provided by this provider. This will be the
On 2013/06/14 00:57:57, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:145: /// within the package, to only return the
files within that subdirectory.
On 2013/06/14 00:57:57, nweiz wrote:
> Does this give access to all files in the package, or just those under an
> "asset" directory? This distinction will need to be clarified before we ship
> this.

All files. This lets it access stuff in asset/ but also stuff in web/ for the
entrypoint package and lib/ since Dart files are assets too.

At least right now, barback is actually pretty pub-agnostic. I'm not sure if
that's a feature or just a coincidence of the current implementation and the
lack of an actual Pub API. It does make it much easier to unit test. :)

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:146: List<String> listFiles(String package, {String
within});
On 2013/06/14 00:57:57, nweiz wrote:
> It seems like this should be "listAssets".

Done. Made it return AssetIds too.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:148: Future<Asset> loadAsset(AssetId id);
On 2013/06/14 00:57:57, nweiz wrote:
> This name is weird, since it doesn't actually load the asset from disk for
> filesystem assets (which I assume is what's going to be loaded when calling
into
> pub). It effectively just returns a path to the asset.

Changed to getAsset() to be a bit more oblique. From the user's perspective it
does "load" it in the sense that they get an object back that they can read
from. It is true that it doesn't cache the contents of the file in memory at the
point in time that you call this. I'm not sure if that's behavior that should be
specified one way or the other.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/06/14 00:57:57, nweiz wrote:
> This should probably be exported somewhere, right?

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.)

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:22: final _resultsController = new
StreamController<ProcessResult>();
On 2013/06/14 00:57:57, nweiz wrote:
> This should be a broadcast controller.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:25: /// /work queue is currently being
processed. Otherwise, it is `null`.
On 2013/06/14 00:57:57, nweiz wrote:
> "/work" -> "work"

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:35: AssetGraph(this._provider,
Iterable<Iterable<Transformer>> transformerPhases) {
On 2013/06/14 00:57:57, nweiz wrote:
> Long line.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:36: // Add phases for each transformer
stage.
On 2013/06/14 00:57:57, nweiz wrote:
> I'm convinced we're going to end up needing semantically-distinct phases.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:43: _phases.add(new _Phase(this,
_phases.length, []));
On 2013/06/14 00:57:57, nweiz wrote:
> This is confusing. What does a phase with no transformers do? Add some more
> explanation.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:52: return _waitForProcess().then((_) {
On 2013/06/14 00:57:57, nweiz wrote:
> Add a TODO to be smarter about only waiting until the asset we need is
> generated.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:55: var node = _phases[i].inputs[id];
On 2013/06/14 00:57:57, nweiz wrote:
> Why are we looking in the phase inputs for the asset output?

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:68: /// Adds [sources] to the graph's known
set of source assets. Will begin
On 2013/06/14 00:57:57, nweiz wrote:
> "Will begin" -> "Begins"

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:70: /// source has already been added, it
is considered modified and all
On 2013/06/14 00:57:57, nweiz wrote:
> "has already been added" -> "is already known"

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:72: void updateSources(Iterable<AssetId>
sources) {
On 2013/06/14 00:57:57, nweiz wrote:
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:87: /// Returns a future that completes
with the background processing is done.
On 2013/06/14 00:57:57, nweiz wrote:
> 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.

Clarified documentation.

> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:121: // the build process.
On 2013/06/14 00:57:57, nweiz wrote:
> 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.

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.

> This warrants a test.

There's a test that multiple calls to updateSources() create a single batch.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:141: void _throwError(Exception error) {
On 2013/06/14 00:57:57, nweiz wrote:
> 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.

Removed this.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:159: class _ChangeBatch {
On 2013/06/14 00:57:57, nweiz wrote:
> It would be nice to split out this and the following classes into separate
files
> as well.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:203: /// The transformers that can use
[assets] as inputs. Their outputs will be
On 2013/06/14 00:57:57, nweiz wrote:
> There's no field named "assets". Did you mean "inputs"?

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:212: /// The transforms currently
applicable on assets in [inputs]. These are the
On 2013/06/14 00:57:57, nweiz wrote:
> "applicable to"

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:220: /// transforms off them.
On 2013/06/14 00:57:57, nweiz wrote:
> This isn't all new nodes in the whole AssetGraph, is it? Isn't it just the
nodes
> that have been added to [inputs]?

Yes, fixed.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:231: void updateInputs(Map<AssetId, Asset>
updated, Set<AssetId> removed) {
On 2013/06/14 00:57:57, nweiz wrote:
> It's weird that this takes [updated] as a map. It feels like an Asset should
> know its id.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:263: return _processTransforms();
On 2013/06/14 00:57:57, nweiz wrote:
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:267: return _processTransforms();
On 2013/06/14 00:57:57, nweiz wrote:
> Style nit: =>

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:301: return
Future.wait(dirtyTransforms.map((node) {
On 2013/06/14 00:57:57, nweiz wrote:
> "node" -> "transform", to avoid it being confused with an AssetNode.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:305: next.updateInputs(updated, removed);
On 2013/06/14 00:57:57, nweiz wrote:
> What about the transform's output? Isn't it also updated?

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:310: /// Represents an asset within the
build dependency graph. It tracks its ID,
On 2013/06/14 00:57:57, nweiz wrote:
> "Represents an asset" is confusing. The Asset class already represents an
asset.

Rewrote.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:311: /// the currently generated actual
asset for it, and any transforms that use
On 2013/06/14 00:57:57, nweiz wrote:
> You're using "it" to refer to two different things in this sentence.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:317: /// The [_TransformNode]s in this
node's phase that consume this asset as an
On 2013/06/14 00:57:57, nweiz wrote:
> "this asset" -> "this node's asset".

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:323: /// Updates this nodes's generated
asset value and marks all transforms that
On 2013/06/14 00:57:57, nweiz wrote:
> "node's"

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:332: class _TransformNode {
On 2013/06/14 00:57:57, nweiz wrote:
> It's pretty confusing right now what the distinction is between this and
> Transform. Document that more thoroughly.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:340: var outputs = new Set<AssetId>();
On 2013/06/14 00:57:57, nweiz wrote:
> What about the non-primary inputs?

Done. Good catch. Added a test.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:358: // TODO(rnystrom): Define what happens
after a collision occurs.
On 2013/06/14 00:57:57, nweiz wrote:
> We should probably have some notion of a node and everything below it being in
> an error state.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:368: removed.addAll(removedOutputs);
On 2013/06/14 00:57:57, nweiz wrote:
> We should be sure we test the case where one transform removes an output at
the
> same time that another one adds that output.

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:374: class _Transform implements Transform
{
On 2013/06/14 00:57:57, nweiz wrote:
> I really hate the pattern of an interface with a single, private
implementation.
> Just call this class Transform and export it.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.dart
File pkg/barback/lib/transformer.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/06/14 00:57:57, nweiz wrote:
> I don't understand why the classes in this file are separate from
barback.dart.

I thought it might be nice to separate out the public APIs for someone just
using barback versus someone implementing their own transformer. If you're just
using barback, you'll only import barback.dart.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:14: /// A [Transformer] represents a processing
step that can take in one or more
On 2013/06/14 00:57:57, nweiz wrote:
> "processing step" here is confusing; it sounds like a transformer is a node in
> the asset graph. It blurs the difference between Transformer, Transform, and
> _TransformNode. Maybe "processor" instead?

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:18: /// files are all examples of transformers.
To define your own transformation
On 2013/06/14 00:57:57, nweiz wrote:
> "a tool to concatenate JS files" is a weird example, since it's unlikely to
ever
> be used by a Dart project.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:19: /// step, you extend (or implement) this
class.
On 2013/06/14 00:57:57, nweiz wrote:
> "you" -> ""

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:22: /// This is used to tell which files this
transformer applies to.
On 2013/06/14 00:57:57, nweiz wrote:
> Explain what a primary input is.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:25: /// Process this transformer on
[transform], which describes the actual
On 2013/06/14 00:57:57, nweiz wrote:
> "Process this transformer on [transform]" is very confusing. What does it mean
> to process a transformer?
> 
> Maybe something like "Run this transformer on the primary input described by
> [transform]."

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:26: /// assets being transformed.
On 2013/06/14 00:57:57, nweiz wrote:
> I don't think it's accurate to say that a Transform "describes the assets
being
> transformed". It only includes the primary asset, and otherwise just provides
> access to additional inputs.
> 
> Also, this documentation should explain how the transformer is supposed to use
> Transform (e.g. call [getInput], do stuff, then call [addOutput]).

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:33: /// one specific usage of it on a set of
files. It is used in [apply()] to
On 2013/06/14 00:57:57, nweiz wrote:
> "usage" -> "instance"
> 
> "It is used" -> "Transformers use a Transform"

Reworded.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:43: /// would be other, secondary inputs.
On 2013/06/14 00:57:57, nweiz wrote:
> "other," -> ""

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/pubspec.yaml
File pkg/barback/pubspec.yaml (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/pubspec.yaml#newc...
pkg/barback/pubspec.yaml:5: scheduled_test: any
On 2013/06/14 00:57:57, nweiz wrote:
> Needs author/homepage/description.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/source_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:9: 
On 2013/06/14 00:57:57, nweiz wrote:
> Unnecessary blank line.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:22: expectAsset(graph,
"app|foo.txt");
On 2013/06/14 00:57:57, nweiz wrote:
> "expectAsset" is a confusing name. I have to look in utils to figure out what
it
> means. "expectAssetExists" would be better,

"exists" is a bit confusing to me because that feels like a weird fit for
generated assets.

> although perhaps "expect(graph, producesAsset(...))" would be best.

That would read OK, but it's using scheduled_test internally, so that would be a
weird fit. expectAsset() gets used pretty frequently, so I thought something
terse would be good.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:25: test("doesn't get an unknown
source", () {
On 2013/06/14 00:57:57, nweiz wrote:
> The term "source" is confusing. I think of packages as the sources of assets.
A
> "source asset" isn't really the source of anything. I get the association with
> "source code", I guess, but I'd rather find a different name. "Concrete
asset"?
> "Initial asset"? "Un-transformed asset"?

I'm not crazy about it either. Maybe "raw" input? I'm definitely up for changing
the name to something better, but if it's OK I'd put that in another patch.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:36: expectNoAsset(graph,
"app|foo.txt");
On 2013/06/14 00:57:57, nweiz wrote:
> Wait, why will this not exist? It's right there!

You have to specifically tell barback "this file exists" (or "was updated"). If
you just request an asset, it won't go straight to the provider to see if it's
there.

We could support that, but I kind of like the idea of users explicitly saying
"here's the set of inputs to work with". It also makes it clearer when
transforms are wired up and to which files. If we just start wiring up
transforms to every single file you can possibly reach from the provider, it
will probably end up doing weird things (like re-processing stuff in output
directories).

Making this explicit lets the provider be simpler.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:54:
graph.updateSources([AssetId.parse("app|foo.txt")]);
On 2013/06/14 00:57:57, nweiz wrote:
> Why is this [updateSources] scheduled, but the previous one is not?

No reason. Removed.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:109: }
On 2013/06/14 00:57:57, nweiz wrote:
> If this is where you're testing add/remove/update behavior, there need to be a
> lot more tests here of situations where the changes happen before, after, and
> during the processing of transformation.

There are some tests for this. Added a couple more. I'm sure there are more
cases I can cover, but it's hard to know which ones I should do.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/transform_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/transform_test.dart:48: test("doesn't find an
output from the same stage", () {
On 2013/06/14 00:57:57, nweiz wrote:
> You're using "phase" and "stage" interchangeably. It's confusing.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/transform_test.dart:58: expectNoAsset(graph,
"app|foo.c");
On 2013/06/14 00:57:57, nweiz wrote:
> Expect that "foo.b" exists.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/transform_test.dart:167: [new
OneToManyTransformer("txt")]
On 2013/06/14 00:57:57, nweiz wrote:
> Can't RewriteTransformer already produce multiple outputs?

Not for a single input. It's a 1-1 transform*er*, it's just that it will have
multiple transforms for multiple different primary inputs.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/transform_test.dart:207: test("doesn't get an
output from a transform whose primary was removed", () {
On 2013/06/14 00:57:57, nweiz wrote:
> "primary" -> "primary input"
> 
> I don't understand this test. There's only one primary input here -- foo.txt
--
> and it's not removed.

Some sort of copy/paste screw up on my part. I think I copied a test, updated
its description, but then didn't actually implement the test. Fixed.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/test_all.dart
File pkg/barback/test/test_all.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/test_all.dar...
pkg/barback/test/test_all.dart:29: }
On 2013/06/14 00:57:57, nweiz wrote:
> What's this file for? The test infrastructure will run the sub-tests on its
own.

Removed it.

nweiz

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#newcode15 pkg/barback/lib/barback.dart:15: /// Parses an [AssetId] string of the form "package|path/to/asset.txt". ...

7 years, 6 months ago (2013-06-18 23:14:45 UTC) #4

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:15: /// Parses an [AssetId] string of the form
"package|path/to/asset.txt".
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Why "|"? Why not ":"?
> 
> I did ":" at first, but I worried that it makes asset IDs look like URLs with
> random schemes, which they are definitely not. This makes it clearer that they
> are different, and gives them a distinct look. At a glance you can see it's a
> package ID and not a path or URL.

I've found the look of these IDs very odd. I was expecting something like
"package:" URIs as IDs for assets in "lib" directories, and "asset:" URIs as IDs
for assets in the "asset" directories. That seems like it would have less
conceptual overhead -- instead of teaching people an entirely new syntax for
identifying assets, we piggyback on the syntax they understand. That also has
the benefit of not requiring people to type "lib" or "asset" all over the place.

On the other hand, I suppose that poses a problem if people want to do
transformations on files in `web` or `bin`. Another possibility would be
"asset:package/path/to/asset.txt", which mirrors "package:" somewhat but
provides access to all files in a package.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:56: /// An identifiable blob of data. Assets may
come from the file system, or
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Clarify what "identifiable" means.
> > 
> > Also, move the second sentence into its own paragraph. Same goes for several
> > other doc comments.
> 
> Done.

Most of these comments still have more than one sentence in their initial
paragraphs. This is contrary to the documentation style guide.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:59: Asset();
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Why does this class have a constructor? It seems like it would work fine as
an
> > interface; there's no behavior we get from extending it.
> 
> Because it has other named constructors, it doesn't get an automatic unnamed
> constructor. That means the subclasses _FileAsset and _StringAsset don't have
a
> superclass constructor to invoke if this is omitted.

But why have subclasses at all? Why not just make it an interface?

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:89: final File _file;
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > I don't like storing a [File] object. I like the pub style of treating the
> > [File] APIs as weird-looking static methods.
> 
> File is just a wrapper around a path. If I don't store it, each method will
just
> create one every time and throw it away. Seemed cleaner this way.

I prefer creating a new [File] each time; I feel like it makes it clearer that
it doesn't have any state other than the path itself. I don't feel especially
strongly about it, though.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:145: /// within the package, to only return the
files within that subdirectory.
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> All files. This lets it access stuff in asset/ but also stuff in web/ for the
> entrypoint package and lib/ since Dart files are assets too.
> 
> At least right now, barback is actually pretty pub-agnostic. I'm not sure if
> that's a feature or just a coincidence of the current implementation and the
> lack of an actual Pub API. It does make it much easier to unit test. :)

My intuition is that we'll eventually have to decide whether to keep it mostly
Pub-agnostic or not; I suspect "not" is the correct direction, since
approximately everyone will be using it with Pub anyway.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:36: // Add phases for each transformer
stage.
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > I'm convinced we're going to end up needing semantically-distinct phases.
> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:52: return _waitForProcess().then((_) {
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Add a TODO to be smarter about only waiting until the asset we need is
> > generated.
> 
> 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.

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.

> 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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:55: var node = _phases[i].inputs[id];
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Why are we looking in the phase inputs for the asset output?
> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:72: void updateSources(Iterable<AssetId>
sources) {
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:87: /// Returns a future that completes
with the background processing is done.
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > 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.
> 
> Clarified documentation.
> 
> > 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.
> 
> 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.

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].

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:231: void updateInputs(Map<AssetId, Asset>
updated, Set<AssetId> removed) {
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > It's weird that this takes [updated] as a map. It feels like an Asset should
> > know its id.
> 
> 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.

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:358: // TODO(rnystrom): Define what happens
after a collision occurs.
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > We should probably have some notion of a node and everything below it being
in
> > an error state.
> 
> 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.

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.

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

For sure, I was just mentioning my thoughts on the question posed by the TODO.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:374: class _Transform implements Transform
{
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > I really hate the pattern of an interface with a single, private
> implementation.
> > Just call this class Transform and export it.
> 
> 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.

"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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.dart
File pkg/barback/lib/transformer.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > I don't understand why the classes in this file are separate from
> barback.dart.
> 
> I thought it might be nice to separate out the public APIs for someone just
> using barback versus someone implementing their own transformer. If you're
just
> using barback, you'll only import barback.dart.

As I mentioned elsewhere, this sort of thing should be done by exporting "src"
files. I'm not sure there's a lot of value in providing this narrow of an
import, though... if barback.dart only adds the name "Barback" to the top-level,
that doesn't seem like something anyone would care about avoiding.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/source_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:22: expectAsset(graph,
"app|foo.txt");
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > "expectAsset" is a confusing name. I have to look in utils to figure out
what
> it
> > means. "expectAssetExists" would be better,
> 
> "exists" is a bit confusing to me because that feels like a weird fit for
> generated assets.
> 
> > although perhaps "expect(graph, producesAsset(...))" would be best.
> 
> That would read OK, but it's using scheduled_test internally, so that would be
a
> weird fit. expectAsset() gets used pretty frequently, so I thought something
> terse would be good.

I don't think it's particularly weird to have a matcher that uses
scheduled_test. The [completes] and [completion] matchers already do.

It's certainly not any clearer that `expectAsset(graph, ...)` uses
scheduled_test than `expect(graph, customMatcher(...))`.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:25: test("doesn't get an unknown
source", () {
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > The term "source" is confusing. I think of packages as the sources of
assets.
> A
> > "source asset" isn't really the source of anything. I get the association
with
> > "source code", I guess, but I'd rather find a different name. "Concrete
> asset"?
> > "Initial asset"? "Un-transformed asset"?
> 
> I'm not crazy about it either. Maybe "raw" input? I'm definitely up for
changing
> the name to something better, but if it's OK I'd put that in another patch.

Sure, another patch is fine.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:36: expectNoAsset(graph,
"app|foo.txt");
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Wait, why will this not exist? It's right there!
> 
> You have to specifically tell barback "this file exists" (or "was updated").
If
> you just request an asset, it won't go straight to the provider to see if it's
> there.
> 
> We could support that, but I kind of like the idea of users explicitly saying
> "here's the set of inputs to work with". It also makes it clearer when
> transforms are wired up and to which files. If we just start wiring up
> transforms to every single file you can possibly reach from the provider, it
> will probably end up doing weird things (like re-processing stuff in output
> directories).
> 
> Making this explicit lets the provider be simpler.

So how does the user specify this? If I run "pub deploy", how do I tell barback
which files I want to be compiled?

In any case, add a comment explaining what's going on here.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/transform_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/transform_test.dart:167: [new
OneToManyTransformer("txt")]
On 2013/06/17 23:35:05, Bob Nystrom wrote:
> On 2013/06/14 00:57:57, nweiz wrote:
> > Can't RewriteTransformer already produce multiple outputs?
> 
> Not for a single input. It's a 1-1 transform*er*, it's just that it will have
> multiple transforms for multiple different primary inputs.

In that case, what's going on in "only runs a transform once for all of its
outputs"?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:11: import '../transformer.dart';
"src" files shouldn't be importing "lib" files. They should be importing the
libraries they need directly.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:74: throw error;
My thought here was that you could *just* throw when you need to report an
error, then have a [catchError] on [getAssetById] that reports the error to
[_resultsController].

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:111: return _processDone =
_process().whenComplete(() {
If you're confident this won't emit errors, why are you using [whenComplete]?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_node.dart:15: /// Keeps a cache of the last built
asset and tracks which transforms depend
I don't understand what "last built asset" means. Is it the last asset that was
ever built? The last asset with the given id that was built? Something else?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_node.dart:21: /// The [_TransformNode]s that consume
this node's asset as an input.
Remove "_"

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_provider.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_provider.dart:14: /// The names of all packages that
can be provided by this provider. This =
Remove "="

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_provider.dart:15: /// will be the transitive
dependency graph of the entrypoint package.
Saying that this will be a graph is a little confusing, since it doesn't
actually return a graph. "The transitive closure of all dependencies of the
entrypoint package" would be more accurate.

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

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:23: /// Building can be interrupted between
phases. For example, an source is added
"a source"

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:24: /// which starts the background process.
Sometime during phase 2 (which is
It's not clear what "the background process" refers to here, or why we're
running phase 2 rather than phase 1.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:33: final int index;
Is this actually used anywhere? It seems weird to store it.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:53: final newInputs = new Set<AssetNode>();
Seems like most of these fields should be private.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:56: Phase next;
It's weird that this is mutable. If you construct phases backwards from last to
first, couldn't you make this final?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:131: (transform) =>
transform.apply())).then((transformOutputs) {
I think this would be a little cleaner formatted like so:

    return Future.wait(dirtyTransforms.map((transform) => transform.apply()))
        .then((transformOutputs) {

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
pkg/barback/lib/src/transform_node.dart:17: /// which primary asset it depends
on.
"primary asset" -> "assets"

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
pkg/barback/lib/src/transform_node.dart:22: var isDirty = true;
These could use at least brief documentation.

Also, it feels like most of these should be private.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
pkg/barback/lib/src/transform_node.dart:42: // This was a broken transformer
doesn't take down the whole graph.
"was" -> "is so"

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/transform...
File pkg/barback/lib/transformer.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/transform...
pkg/barback/lib/transformer.dart:53: /// only during an actual transform
application to to facilitate communication
"to to" -> "to"

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/pubspec.yaml
File pkg/barback/pubspec.yaml (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/pubspec.yaml#...
pkg/barback/pubspec.yaml:5: An asset build system.
Indent one more space.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
File pkg/barback/test/asset_graph/errors_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:27: expectCollision(graph,
"app|foo.b");
I really think expect(graph, hasCollision("app|foo.b")) would read a lot better
here.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:36: expect(result.error is
AssetNotFoundException, isTrue);
expect(result.error, new isInstanceOf<AssetNotFoundException>())

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:39: }));
This wrapAsync/gotError stuff is complicated and gross. Why not do this:

    expect(schedule(() => graph.results.single).then((result) {
      expect(...);
    }), completes);

?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:88: }));
It's confusing that you're setting this up before the call that *isn't* expected
to have an error, rather than before the one that *is*.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:134: // to transform a file,
should we just skip past it to the source?
Definitely not. If a transformer fails, getting the failed output asset should
expose that failure.

In general, once a preprocessor-phase transformer has been run on an asset, that
asset should no longer be available.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:151: var resultFuture =
graph.results.first;
Why are you getting this future here? Why not do it in schedule()?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:163: });
I think wrapping the first argument to [expect] in [schedule] and using
[completion] is a little cleaner than wrapping an entire [then] call in
[schedule].

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
File pkg/barback/test/asset_graph/transform_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/transform_test.dart:198: // Remove a non-primary
input.
"Remove" is a little confusing here, since you're not calling [removeSources].

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/transform_test.dart:262: test("doesn't get an
output from a transform whose primary is removed", () {
"primary" -> "primary input"

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_id...
File pkg/barback/test/asset_id_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_id...
pkg/barback/test/asset_id_test.dart:25: });
Other edge cases: empty package string, empty path string.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/utils.dart
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:4: library barback.test.utils;
Nit: extra newline before this. Also in other test files.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:30: expect(asset is MockAsset, isTrue);
expect(asset, new isInstanceOf<MockAsset>())

Bob Nystrom

Thanks! https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart File pkg/barback/lib/barback.dart (right): https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#newcode15 pkg/barback/lib/barback.dart:15: /// Parses an [AssetId] string of the form ...

7 years, 6 months ago (2013-06-20 00:23:59 UTC) #5

Thanks!

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:15: /// Parses an [AssetId] string of the form
"package|path/to/asset.txt".
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Why "|"? Why not ":"?
> > 
> > I did ":" at first, but I worried that it makes asset IDs look like URLs
with
> > random schemes, which they are definitely not. This makes it clearer that
they
> > are different, and gives them a distinct look. At a glance you can see it's
a
> > package ID and not a path or URL.
> 
> I've found the look of these IDs very odd. I was expecting something like
> "package:" URIs as IDs for assets in "lib" directories, and "asset:" URIs as
IDs
> for assets in the "asset" directories. That seems like it would have less
> conceptual overhead -- instead of teaching people an entirely new syntax for
> identifying assets, we piggyback on the syntax they understand. That also has
> the benefit of not requiring people to type "lib" or "asset" all over the
place.
> 
> On the other hand, I suppose that poses a problem if people want to do
> transformations on files in `web` or `bin`. Another possibility would be
> "asset:package/path/to/asset.txt", which mirrors "package:" somewhat but
> provides access to all files in a package.

At least right now, this method is only really being used by tests. (At one
point, it was just a function in the test utils code.) I wasn't intending this
to be a very user-visible syntax for assets.

I realize that we do need that syntax for things like referencing assets in
non-Dart files, but it wasn't my overt intent to use this for that. I just
needed something terse so that I could create ids in tests and print them to
debug my code.

I do kind of like the way these ones look, but I'm up for iterating on them in
later patches. This definitely isn't carved in stone.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:56: /// An identifiable blob of data. Assets may
come from the file system, or
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Clarify what "identifiable" means.
> > > 
> > > Also, move the second sentence into its own paragraph. Same goes for
several
> > > other doc comments.
> > 
> > Done.
> 
> Most of these comments still have more than one sentence in their initial
> paragraphs. This is contrary to the documentation style guide.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:59: Asset();
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Why does this class have a constructor? It seems like it would work fine
as
> an
> > > interface; there's no behavior we get from extending it.
> > 
> > Because it has other named constructors, it doesn't get an automatic unnamed
> > constructor. That means the subclasses _FileAsset and _StringAsset don't
have
> a
> > superclass constructor to invoke if this is omitted.
> 
> But why have subclasses at all? Why not just make it an interface?

Oh, duh. Right. I think at some point there used to be a concrete inherited
method or two but not anymore. Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:89: final File _file;
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > I don't like storing a [File] object. I like the pub style of treating the
> > > [File] APIs as weird-looking static methods.
> > 
> > File is just a wrapper around a path. If I don't store it, each method will
> just
> > create one every time and throw it away. Seemed cleaner this way.
> 
> I prefer creating a new [File] each time; I feel like it makes it clearer that
> it doesn't have any state other than the path itself. I don't feel especially
> strongly about it, though.

I kind of like it this way. I'll leave it for now but I'm up for changing it if
it gets unwieldy.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:145: /// within the package, to only return the
files within that subdirectory.
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > All files. This lets it access stuff in asset/ but also stuff in web/ for
the
> > entrypoint package and lib/ since Dart files are assets too.
> > 
> > At least right now, barback is actually pretty pub-agnostic. I'm not sure if
> > that's a feature or just a coincidence of the current implementation and the
> > lack of an actual Pub API. It does make it much easier to unit test. :)
> 
> My intuition is that we'll eventually have to decide whether to keep it mostly
> Pub-agnostic or not; I suspect "not" is the correct direction, since
> approximately everyone will be using it with Pub anyway.

Agreed. I don't intend to try hard to keep it untainted by pub. I just want it
loosely coupled enough to be able to test easily.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:36: // Add phases for each transformer
stage.
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > I'm convinced we're going to end up needing semantically-distinct phases.
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:52: return _waitForProcess().then((_) {
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Add a TODO to be smarter about only waiting until the asset we need is
> > > generated.
> > 
> > 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.
> 
> 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.
> 
> > 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.
> 

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:55: var node = _phases[i].inputs[id];
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Why are we looking in the phase inputs for the asset output?
> > 
> > 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.
> 
> 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.

Done.

> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:72: void updateSources(Iterable<AssetId>
sources) {
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:87: /// Returns a future that completes
with the background processing is done.
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > 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.
> > 
> > Clarified documentation.
> > 
> > > 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.
> > 
> > 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.
> 
> 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].

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:231: void updateInputs(Map<AssetId, Asset>
updated, Set<AssetId> removed) {
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > It's weird that this takes [updated] as a map. It feels like an Asset
should
> > > know its id.
> > 
> > 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.
> 
> Let's postpone the decision until we have a better idea of how Asset and
AssetId
> are used in the system as a whole.

SGTM.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:346: /// this time will be added to
[removed].
On 2013/06/14 00:57:57, nweiz wrote:
> 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.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:358: // TODO(rnystrom): Define what happens
after a collision occurs.
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > We should probably have some notion of a node and everything below it
being
> in
> > > an error state.
> > 
> > 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.
> 
> 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.
> 
> > But I'd like to do that in a separate patch just to keep this
simpler/smaller
> > for now.
> 
> For sure, I was just mentioning my thoughts on the question posed by the TODO.

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:374: class _Transform implements Transform
{
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > I really hate the pattern of an interface with a single, private
> > implementation.
> > > Just call this class Transform and export it.
> > 
> > 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.
> 
> "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".

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

> Everything in "lib/src" should import
> only from "lib/src"; every external library should import only from "lib".

Done.

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

Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.dart
File pkg/barback/lib/transformer.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/transformer.d...
pkg/barback/lib/transformer.dart:1: // Copyright (c) 2013, the Dart project
authors.  Please see the AUTHORS file
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > I don't understand why the classes in this file are separate from
> > barback.dart.
> > 
> > I thought it might be nice to separate out the public APIs for someone just
> > using barback versus someone implementing their own transformer. If you're
> just
> > using barback, you'll only import barback.dart.
> 
> As I mentioned elsewhere, this sort of thing should be done by exporting "src"
> files. I'm not sure there's a lot of value in providing this narrow of an
> import, though... if barback.dart only adds the name "Barback" to the
top-level,
> that doesn't seem like something anyone would care about avoiding.

OK. Done.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/source_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:22: expectAsset(graph,
"app|foo.txt");
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > "expectAsset" is a confusing name. I have to look in utils to figure out
> what
> > it
> > > means. "expectAssetExists" would be better,
> > 
> > "exists" is a bit confusing to me because that feels like a weird fit for
> > generated assets.
> > 
> > > although perhaps "expect(graph, producesAsset(...))" would be best.
> > 
> > That would read OK, but it's using scheduled_test internally, so that would
be
> a
> > weird fit. expectAsset() gets used pretty frequently, so I thought something
> > terse would be good.
> 
> I don't think it's particularly weird to have a matcher that uses
> scheduled_test. The [completes] and [completion] matchers already do.
> 
> It's certainly not any clearer that `expectAsset(graph, ...)` uses
> scheduled_test than `expect(graph, customMatcher(...))`.

I've got some other test refactoring I want to do after this goes in (to
minimize churn on what you have to re-review). I'm up for improving this then.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:36: expectNoAsset(graph,
"app|foo.txt");
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Wait, why will this not exist? It's right there!
> > 
> > You have to specifically tell barback "this file exists" (or "was updated").
> If
> > you just request an asset, it won't go straight to the provider to see if
it's
> > there.
> > 
> > We could support that, but I kind of like the idea of users explicitly
saying
> > "here's the set of inputs to work with". It also makes it clearer when
> > transforms are wired up and to which files. If we just start wiring up
> > transforms to every single file you can possibly reach from the provider, it
> > will probably end up doing weird things (like re-processing stuff in output
> > directories).
> > 
> > Making this explicit lets the provider be simpler.
> 
> So how does the user specify this? If I run "pub deploy", how do I tell
barback
> which files I want to be compiled?
> 
> In any case, add a comment explaining what's going on here.

Pub deploy will automatically call updateSources() on everything that's in your
web directory. (But note that it's *provider* will let you access stuff in web,
lib, etc.)

If you're implementing a push-based build step using barback, that's the basic
model:

1. update all of the source inputs in one lump
2. wait for the build to complete
3. gather all of the output assets and write them to disk

(Of course, all of those still need more fleshing out right now, but I'm working
on it.)

Added a comment.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/transform_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/transform_test.dart:167: [new
OneToManyTransformer("txt")]
On 2013/06/18 23:14:45, nweiz wrote:
> On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > On 2013/06/14 00:57:57, nweiz wrote:
> > > Can't RewriteTransformer already produce multiple outputs?
> > 
> > Not for a single input. It's a 1-1 transform*er*, it's just that it will
have
> > multiple transforms for multiple different primary inputs.
> 
> In that case, what's going on in "only runs a transform once for all of its
> outputs"?

Oh, right. I forgot I added support for outputting to multiple extensions with
that. They do slightly different things. RewriteTransformer can have multiple
outputs, but that's a property of the *transformer* itself. If you create a
RewriteTransformer with two extensions, it will always output two files for each
input.

OneToManyTransformer() uses the *contents* of the primary input to determine the
outputs. That means you can change what it outputs in the middle of a test by
updating the primary input file.

For this specific test, I could have used either of those mock transformers.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:11: import '../transformer.dart';
On 2013/06/18 23:14:46, nweiz wrote:
> "src" files shouldn't be importing "lib" files. They should be importing the
> libraries they need directly.

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:74: throw error;
On 2013/06/18 23:14:46, nweiz wrote:
> My thought here was that you could *just* throw when you need to report an
> error, then have a [catchError] on [getAssetById] that reports the error to
> [_resultsController].

In most cases, though, we don't want to just unwind to the top of the public API
and bail on an error. I intend barback to be resilient and catch those
internally where it can do something more appropriate.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:111: return _processDone =
_process().whenComplete(() {
On 2013/06/18 23:14:46, nweiz wrote:
> If you're confident this won't emit errors, why are you using [whenComplete]?

Why not?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_node.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_node.dart:15: /// Keeps a cache of the last built
asset and tracks which transforms depend
On 2013/06/18 23:14:46, nweiz wrote:
> I don't understand what "last built asset" means. Is it the last asset that
was
> ever built? The last asset with the given id that was built? Something else?

Clarified.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_node.dart:21: /// The [_TransformNode]s that consume
this node's asset as an input.
On 2013/06/18 23:14:46, nweiz wrote:
> Remove "_"

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_provider.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_provider.dart:14: /// The names of all packages that
can be provided by this provider. This =
On 2013/06/18 23:14:46, nweiz wrote:
> Remove "="

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_provider.dart:15: /// will be the transitive
dependency graph of the entrypoint package.
On 2013/06/18 23:14:46, nweiz wrote:
> Saying that this will be a graph is a little confusing, since it doesn't
> actually return a graph. "The transitive closure of all dependencies of the
> entrypoint package" would be more accurate.

Done.

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

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:23: /// Building can be interrupted between
phases. For example, an source is added
On 2013/06/18 23:14:46, nweiz wrote:
> "a source"

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:24: /// which starts the background process.
Sometime during phase 2 (which is
On 2013/06/18 23:14:46, nweiz wrote:
> It's not clear what "the background process" refers to here, or why we're
> running phase 2 rather than phase 1.

Arbitrary number for the example.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:33: final int index;
On 2013/06/18 23:14:46, nweiz wrote:
> Is this actually used anywhere? It seems weird to store it.

I end up using it for debug printing. I can remove it if you like, but it's kind
of handy. At some point, I'll put a decent toString() on this.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:53: final newInputs = new Set<AssetNode>();
On 2013/06/18 23:14:46, nweiz wrote:
> Seems like most of these fields should be private.

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:56: Phase next;
On 2013/06/18 23:14:46, nweiz wrote:
> It's weird that this is mutable. If you construct phases backwards from last
to
> first, couldn't you make this final?

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/phase...
pkg/barback/lib/src/phase.dart:131: (transform) =>
transform.apply())).then((transformOutputs) {
On 2013/06/18 23:14:46, nweiz wrote:
> I think this would be a little cleaner formatted like so:
> 
>     return Future.wait(dirtyTransforms.map((transform) => transform.apply()))
>         .then((transformOutputs) {

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
pkg/barback/lib/src/transform_node.dart:17: /// which primary asset it depends
on.
On 2013/06/18 23:14:46, nweiz wrote:
> "primary asset" -> "assets"

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
pkg/barback/lib/src/transform_node.dart:22: var isDirty = true;
On 2013/06/18 23:14:46, nweiz wrote:
> These could use at least brief documentation.
> 
> Also, it feels like most of these should be private.

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/trans...
pkg/barback/lib/src/transform_node.dart:42: // This was a broken transformer
doesn't take down the whole graph.
On 2013/06/18 23:14:46, nweiz wrote:
> "was" -> "is so"

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/transform...
File pkg/barback/lib/transformer.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/transform...
pkg/barback/lib/transformer.dart:53: /// only during an actual transform
application to to facilitate communication
On 2013/06/18 23:14:46, nweiz wrote:
> "to to" -> "to"

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/pubspec.yaml
File pkg/barback/pubspec.yaml (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/pubspec.yaml#...
pkg/barback/pubspec.yaml:5: An asset build system.
On 2013/06/18 23:14:46, nweiz wrote:
> Indent one more space.

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
File pkg/barback/test/asset_graph/errors_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:27: expectCollision(graph,
"app|foo.b");
On 2013/06/18 23:14:46, nweiz wrote:
> I really think expect(graph, hasCollision("app|foo.b")) would read a lot
better
> here.

My plan is to have utils.dart keep track of the graph directly. (It needs to do
that to register the build result listener early enough to ensure results aren't
dropped). That means you won't have to keep passing it in. At that point, I
think dedicated expectation functions are nice.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:36: expect(result.error is
AssetNotFoundException, isTrue);
On 2013/06/18 23:14:46, nweiz wrote:
> expect(result.error, new isInstanceOf<AssetNotFoundException>())

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:39: }));
On 2013/06/18 23:14:46, nweiz wrote:
> This wrapAsync/gotError stuff is complicated and gross. Why not do this:
> 
>     expect(schedule(() => graph.results.single).then((result) {
>       expect(...);
>     }), completes);
> 
> ?

Added a TODO. I'm planning to refactor it completely after this lands.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:88: }));
On 2013/06/18 23:14:46, nweiz wrote:
> It's confusing that you're setting this up before the call that *isn't*
expected
> to have an error, rather than before the one that *is*.

This is a broadcast stream and the build process runs in the background. That
means as soon as you call updateSources(), it can start generating results. I
you don't start listening early, it will drop them on the floor.

This will get less nasty when I have a chance to refactor it.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:134: // to transform a file,
should we just skip past it to the source?
On 2013/06/18 23:14:46, nweiz wrote:
> Definitely not. If a transformer fails, getting the failed output asset should
> expose that failure.
> 
> In general, once a preprocessor-phase transformer has been run on an asset,
that
> asset should no longer be available.

I'm going to leave this test for now just to pin down the current behavior, but
I agree one thing I'll be working on soon is a coherent failure handling
strategy. I expect this test to change then.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:151: var resultFuture =
graph.results.first;
On 2013/06/18 23:14:46, nweiz wrote:
> Why are you getting this future here? Why not do it in schedule()?

I need to make sure I grab it before the updateSources() call so the results
don't get discarded.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:163: });
On 2013/06/18 23:14:46, nweiz wrote:
> I think wrapping the first argument to [expect] in [schedule] and using
> [completion] is a little cleaner than wrapping an entire [then] call in
> [schedule].

Maybe it's just me, but I really prefer the schedule() calls to be at the top
level of the method. I think of them as imperative statements and not embeddable
expressions.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
File pkg/barback/test/asset_graph/transform_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/transform_test.dart:198: // Remove a non-primary
input.
On 2013/06/18 23:14:46, nweiz wrote:
> "Remove" is a little confusing here, since you're not calling [removeSources].


Reworded.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/transform_test.dart:262: test("doesn't get an
output from a transform whose primary is removed", () {
On 2013/06/18 23:14:46, nweiz wrote:
> "primary" -> "primary input"

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_id...
File pkg/barback/test/asset_id_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_id...
pkg/barback/test/asset_id_test.dart:25: });
On 2013/06/18 23:14:46, nweiz wrote:
> Other edge cases: empty package string, empty path string.

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/utils.dart
File pkg/barback/test/utils.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:4: library barback.test.utils;
On 2013/06/18 23:14:46, nweiz wrote:
> Nit: extra newline before this. Also in other test files.

Done.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/utils.da...
pkg/barback/test/utils.dart:30: expect(asset is MockAsset, isTrue);
On 2013/06/18 23:14:46, nweiz wrote:
> expect(asset, new isInstanceOf<MockAsset>())

Done.

nweiz

I still want to be sure we avoid top-levelling errors, but other than that LGTM. ...

7 years, 6 months ago (2013-06-20 23:06:08 UTC) #6

I still want to be sure we avoid top-levelling errors, but other than that LGTM.

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/barback.dart#...
pkg/barback/lib/barback.dart:15: /// Parses an [AssetId] string of the form
"package|path/to/asset.txt".
On 2013/06/20 00:23:59, Bob Nystrom wrote:
> On 2013/06/18 23:14:45, nweiz wrote:
> > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > Why "|"? Why not ":"?
> > > 
> > > I did ":" at first, but I worried that it makes asset IDs look like URLs
> with
> > > random schemes, which they are definitely not. This makes it clearer that
> they
> > > are different, and gives them a distinct look. At a glance you can see
it's
> a
> > > package ID and not a path or URL.
> > 
> > I've found the look of these IDs very odd. I was expecting something like
> > "package:" URIs as IDs for assets in "lib" directories, and "asset:" URIs as
> IDs
> > for assets in the "asset" directories. That seems like it would have less
> > conceptual overhead -- instead of teaching people an entirely new syntax for
> > identifying assets, we piggyback on the syntax they understand. That also
has
> > the benefit of not requiring people to type "lib" or "asset" all over the
> place.
> > 
> > On the other hand, I suppose that poses a problem if people want to do
> > transformations on files in `web` or `bin`. Another possibility would be
> > "asset:package/path/to/asset.txt", which mirrors "package:" somewhat but
> > provides access to all files in a package.
> 
> At least right now, this method is only really being used by tests. (At one
> point, it was just a function in the test utils code.) I wasn't intending this
> to be a very user-visible syntax for assets.
> 
> I realize that we do need that syntax for things like referencing assets in
> non-Dart files, but it wasn't my overt intent to use this for that. I just
> needed something terse so that I could create ids in tests and print them to
> debug my code.
> 
> I do kind of like the way these ones look, but I'm up for iterating on them in
> later patches. This definitely isn't carved in stone.

Okay, sounds good.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:36: // Add phases for each transformer
stage.
On 2013/06/20 00:23:59, Bob Nystrom wrote:
> On 2013/06/18 23:14:45, nweiz wrote:
> > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > I'm convinced we're going to end up needing semantically-distinct
phases.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:87: /// Returns a future that completes
with the background processing is done.
On 2013/06/20 00:23:59, Bob Nystrom wrote:
> On 2013/06/18 23:14:45, nweiz wrote:
> > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > 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.
> > > 
> > > Clarified documentation.
> > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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].
> 
> 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.

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].

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/source_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:36: expectNoAsset(graph,
"app|foo.txt");
On 2013/06/20 00:23:59, Bob Nystrom wrote:
> On 2013/06/18 23:14:45, nweiz wrote:
> > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > Wait, why will this not exist? It's right there!
> > > 
> > > You have to specifically tell barback "this file exists" (or "was
updated").
> > If
> > > you just request an asset, it won't go straight to the provider to see if
> it's
> > > there.
> > > 
> > > We could support that, but I kind of like the idea of users explicitly
> saying
> > > "here's the set of inputs to work with". It also makes it clearer when
> > > transforms are wired up and to which files. If we just start wiring up
> > > transforms to every single file you can possibly reach from the provider,
it
> > > will probably end up doing weird things (like re-processing stuff in
output
> > > directories).
> > > 
> > > Making this explicit lets the provider be simpler.
> > 
> > So how does the user specify this? If I run "pub deploy", how do I tell
> barback
> > which files I want to be compiled?
> > 
> > In any case, add a comment explaining what's going on here.
> 
> Pub deploy will automatically call updateSources() on everything that's in
your
> web directory. (But note that it's *provider* will let you access stuff in
web,
> lib, etc.)
> 
> If you're implementing a push-based build step using barback, that's the basic
> model:
> 
> 1. update all of the source inputs in one lump
> 2. wait for the build to complete
> 3. gather all of the output assets and write them to disk
> 
> (Of course, all of those still need more fleshing out right now, but I'm
working
> on it.)
> 
> Added a comment.

It seems a little weird that the set of available sources comes from the
provider, but the set of sources that actually (might) need compiling comes from
whoever's calling barback.

Do you have a test for what happens if [updateSources] is called with an id that
the provider doesn't provide?

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:111: return _processDone =
_process().whenComplete(() {
On 2013/06/20 00:23:59, Bob Nystrom wrote:
> On 2013/06/18 23:14:46, nweiz wrote:
> > If you're confident this won't emit errors, why are you using
[whenComplete]?
> 
> Why not?

Because functionally it's identical to [then], but it communicates to a reader
that you expect that an error might occur here.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
File pkg/barback/test/asset_graph/errors_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:163: });
On 2013/06/20 00:23:59, Bob Nystrom wrote:
> On 2013/06/18 23:14:46, nweiz wrote:
> > I think wrapping the first argument to [expect] in [schedule] and using
> > [completion] is a little cleaner than wrapping an entire [then] call in
> > [schedule].
> 
> Maybe it's just me, but I really prefer the schedule() calls to be at the top
> level of the method. I think of them as imperative statements and not
embeddable
> expressions.

I disagree. I like them best when they're inside library methods, but failing
that I'd rather it look like an inline library method than a series of
large-looking blocks. Using expect(..., completion(...)) can also produce nicer
output and error detection in many cases.

I also really don't like having to remember to return the inner future.

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_graph.dart:53: for (var i = transformerPhases.length -
1; i >= 0; i--) {
List has a [reversed] getter these days :).

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_graph.dart:81: // TODO(rnystrom): Currently does not
omit assets that are actually used
Style nit: I like separating informative comments from TODO comments with an
empty line.

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_graph.dart:82: // as inputs for transformers. This
means you can request and get a
"a an" -> "an"

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/transf...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transform.dart:22: new Transform._(node, inputs, outputs);
In ScheduledTest, I just bit the bullet and exposed some constructors I didn't
intend anyone to use. I feel like the code ended up cleaner that way. YMMV.

Bob Nystrom

Thanks! https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_graph.dart File pkg/barback/lib/src/asset_graph.dart (right): https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_graph.dart#newcode36 pkg/barback/lib/src/asset_graph.dart:36: // Add phases for each transformer stage. On ...

7 years, 6 months ago (2013-06-21 00:13:20 UTC) #7

Thanks!

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:36: // Add phases for each transformer
stage.
On 2013/06/20 23:06:08, nweiz wrote:
> On 2013/06/20 00:23:59, Bob Nystrom wrote:
> > On 2013/06/18 23:14:45, nweiz wrote:
> > > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > > I'm convinced we're going to end up needing semantically-distinct
> phases.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/16854005/diff/1/pkg/barback/lib/src/asset_gra...
pkg/barback/lib/src/asset_graph.dart:87: /// Returns a future that completes
with the background processing is done.
On 2013/06/20 23:06:08, nweiz wrote:
> On 2013/06/20 00:23:59, Bob Nystrom wrote:
> > On 2013/06/18 23:14:45, nweiz wrote:
> > > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > > 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.
> > > > 
> > > > Clarified documentation.
> > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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].
> > 
> > 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.
> 
> 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].

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

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
File pkg/barback/test/asset_graph/source_test.dart (right):

https://codereview.chromium.org/16854005/diff/1/pkg/barback/test/asset_graph/...
pkg/barback/test/asset_graph/source_test.dart:36: expectNoAsset(graph,
"app|foo.txt");
On 2013/06/20 23:06:08, nweiz wrote:
> On 2013/06/20 00:23:59, Bob Nystrom wrote:
> > On 2013/06/18 23:14:45, nweiz wrote:
> > > On 2013/06/17 23:35:05, Bob Nystrom wrote:
> > > > On 2013/06/14 00:57:57, nweiz wrote:
> > > > > Wait, why will this not exist? It's right there!
> > > > 
> > > > You have to specifically tell barback "this file exists" (or "was
> updated").
> > > If
> > > > you just request an asset, it won't go straight to the provider to see
if
> > it's
> > > > there.
> > > > 
> > > > We could support that, but I kind of like the idea of users explicitly
> > saying
> > > > "here's the set of inputs to work with". It also makes it clearer when
> > > > transforms are wired up and to which files. If we just start wiring up
> > > > transforms to every single file you can possibly reach from the
provider,
> it
> > > > will probably end up doing weird things (like re-processing stuff in
> output
> > > > directories).
> > > > 
> > > > Making this explicit lets the provider be simpler.
> > > 
> > > So how does the user specify this? If I run "pub deploy", how do I tell
> > barback
> > > which files I want to be compiled?
> > > 
> > > In any case, add a comment explaining what's going on here.
> > 
> > Pub deploy will automatically call updateSources() on everything that's in
> your
> > web directory. (But note that it's *provider* will let you access stuff in
> web,
> > lib, etc.)
> > 
> > If you're implementing a push-based build step using barback, that's the
basic
> > model:
> > 
> > 1. update all of the source inputs in one lump
> > 2. wait for the build to complete
> > 3. gather all of the output assets and write them to disk
> > 
> > (Of course, all of those still need more fleshing out right now, but I'm
> working
> > on it.)
> > 
> > Added a comment.
> 
> It seems a little weird that the set of available sources comes from the
> provider, but the set of sources that actually (might) need compiling comes
from
> whoever's calling barback.

The code calling barback also is responsible for giving it a provider, so I
think this ends up working out.

> 
> Do you have a test for what happens if [updateSources] is called with an id
that
> the provider doesn't provide?

I do now. It behaves like a missing input for a transformer.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/lib/src/asset...
pkg/barback/lib/src/asset_graph.dart:111: return _processDone =
_process().whenComplete(() {
On 2013/06/20 23:06:08, nweiz wrote:
> On 2013/06/20 00:23:59, Bob Nystrom wrote:
> > On 2013/06/18 23:14:46, nweiz wrote:
> > > If you're confident this won't emit errors, why are you using
> [whenComplete]?
> > 
> > Why not?
> 
> Because functionally it's identical to [then], but it communicates to a reader
> that you expect that an error might occur here.

Added a catchError() too here like you suggested.

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
File pkg/barback/test/asset_graph/errors_test.dart (right):

https://codereview.chromium.org/16854005/diff/19001/pkg/barback/test/asset_gr...
pkg/barback/test/asset_graph/errors_test.dart:163: });
On 2013/06/20 23:06:08, nweiz wrote:
> On 2013/06/20 00:23:59, Bob Nystrom wrote:
> > On 2013/06/18 23:14:46, nweiz wrote:
> > > I think wrapping the first argument to [expect] in [schedule] and using
> > > [completion] is a little cleaner than wrapping an entire [then] call in
> > > [schedule].
> > 
> > Maybe it's just me, but I really prefer the schedule() calls to be at the
top
> > level of the method. I think of them as imperative statements and not
> embeddable
> > expressions.
> 
> I disagree. I like them best when they're inside library methods, but failing
> that I'd rather it look like an inline library method than a series of
> large-looking blocks. Using expect(..., completion(...)) can also produce
nicer
> output and error detection in many cases.
> 
> I also really don't like having to remember to return the inner future.

My plan for the next patch is to clean up the tests, so these will be moved into
library methods. I agree this is ugly.

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
File pkg/barback/lib/src/asset_graph.dart (right):

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_graph.dart:53: for (var i = transformerPhases.length -
1; i >= 0; i--) {
On 2013/06/20 23:06:08, nweiz wrote:
> List has a [reversed] getter these days :).

Done.

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_graph.dart:81: // TODO(rnystrom): Currently does not
omit assets that are actually used
On 2013/06/20 23:06:08, nweiz wrote:
> Style nit: I like separating informative comments from TODO comments with an
> empty line.

Done.

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/asset_...
pkg/barback/lib/src/asset_graph.dart:82: // as inputs for transformers. This
means you can request and get a
On 2013/06/20 23:06:08, nweiz wrote:
> "a an" -> "an"

Done.

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/transf...
File pkg/barback/lib/src/transform.dart (right):

https://codereview.chromium.org/16854005/diff/6010/pkg/barback/lib/src/transf...
pkg/barback/lib/src/transform.dart:22: new Transform._(node, inputs, outputs);
On 2013/06/20 23:06:08, nweiz wrote:
> In ScheduledTest, I just bit the bullet and exposed some constructors I didn't
> intend anyone to use. I feel like the code ended up cleaner that way. YMMV.

This is a little more bullet-proof. Since this is an API that end users will
consume in non-test code, I like being as tight as possible. A bit
boilerplate-y, but I think it's actually kind of a neat trick.

Bob Nystrom

7 years, 6 months ago (2013-06-21 00:22:00 UTC) #8

Message was sent while issue was closed.

Committed patchset #7 manually as r24257 (presubmit successful).

Issue 16854005: First pass at build dependency graph for barback. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Remove in-progress stuff. #

Patch Set 3 : Remove more in-progress stuff from this patch. #

Patch Set 4 : Test that transforms in the same phase run in parallel. #

Patch Set 5 : Revise. #

Patch Set 6 : Revise. #

Patch Set 7 : Handle unprovided sources and errors a bit better. #

Messages