Support loading transformer plugins from pub.

Support loading transformer plugins from pub.

This is still missing some functionality, most notably the ability for
a transformer to import transformed code.

R=rnystrom@google.com
BUG=

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

[Bob Nystrom, 2013-08-27 22:12:30]

I'm uncomfortably excited about this! :)

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:26: watchSources(graph, barback);
I think it's cleaner to do this after the server is bound. Previously, I had
some race conditions in the tests because notifications could come in before the
server had finished starting up, which made the stdout non-deterministic.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:32: // non-programmatic barback
errors, they should take down the whole program.
"non-programmatic" -> "programmatic"?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:72: var idsToPackages = new
Map<AssetId, Set<String>>();
Document this, maybe:

// Map each transformer to the names of the packages that use it.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:78: var transformersForId = new
Map<AssetId, Set<Transformer>>();
Nit: move to just above Future.wait.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:47: return
mirrors.libraries[uri].classes.values.where((mirror) {
"mirror" -> "classMirror".

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:48: if
(mirror.isPrivate) return false;
Might want to check and see if the class is abstract too.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:52: if
(!constructor.parameters.isEmpty) return false;
Add a TODO to report this error to the user.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:105: /// Returns
whether or not [superclass] is a superclass of [mirror].
Clearer would be "[mirror] is an instance of [superclass]."

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:116: Map
_wrapTransformer(Transformer transformer) {
"wrap" -> "serialize", "unwrap" -> "deserialize" here and elsewhere.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:140: return new
Asset.fromStream(_unwrapId(asset['id']), box.stream);
I really think we should serialize assets directly. I would add static
serialize() and deserialize() methods to Asset that handle the different know
Asset classes. Then add a TODO to handle unknown asset types.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:144: AssetId
_unwrapId(Map id) => new AssetId(id['package'], id['path']);
I'd prefer adding a serialize() method directly on AssetId and an
AssetId.deserialize() constructor. In both cases, make the data type just Object
and not Map.

Or we could use the serialization package.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:179: /// Sends the
result of [_sendFuture] from [portCall], which should be the
"Sends" -> "Receives".

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:256:
_TRANSFORMER_ISOLATE.replaceAll('<<PORT>>', server.port.toString());
Add a TODO to do something less unholy here...

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:268: /// The port
with which we communicate with the host isolate.
"host" -> "child"?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/pub_package_provider.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/pub_package_provider.dart:15: /// can
assets within pub packages.
"can assets" -> "can find assets", though the former has a certain charm. Do you
even asset, bro?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:11: ///
the barback server rather than to pub's package root.
So meta.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:13:
RewriteImportTransformer();
Remove this?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:34: index
= directive.uri.literal.end;
Very nice.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:44: /// A
simple visitor that collects import and export nodes.
What about parts?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:48:
_ImportExportCollector();
Remove this?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:42: /// This server will serve
assets from [barback], and use [entrypoint] as the
"entrypoint" -> "rootPackage".

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:68:
_methodNotAllowed(request);
Should add a test for this.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:74: /// Returns a Future that will
resolve to a send point that will communicate to
"send point" -> "[SendPort]".

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:130:
CrossIsolateException.deserialize(Map error)
Instead of Map here and below, I'd just use Object to completely encapsulate the
serialized type.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:192: Future<PackageGraph>
loadPackageGraph() {
I wonder if this and lockFile should just be getters. Is there a reason for them
not to be now that they are both sync?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:185: throw new
FormatException('Transformers "$transformers" must be a list.');
'The pubspec field "transformers" must be a list, but was "$transformers".'

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:196: var parts = split1(transformer,
"/");
Document this chunk of code.

Also, can you file a bug to document the new pubspec changes on
pub.dartlang.org?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:202: throw new
FormatException('Transformer "$transformer" not found.');
Clarify this message, like 'Could not find package for transformer
"$transformer".'

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/fails_to_load_a_file_that_defines_no_transforms_test.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_file_that_defines_no_transforms_test.dart:21:
d.dir("lib", [d.dir("src", [
Move d.dir("src") to its own line and indent the next line to make the nesting a
bit clearer please. (Or just put transformer.dart in lib.)

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_file_that_defines_no_transforms_test.dart:31:
expect(pub.nextErrLine(), completion(startsWith('required by myapp.')));
Why not just move these two lines after shouldExit(1) and then do a single test
against remainingStderr()?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_from_a_non_dependency.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_from_a_non_dependency.dart:23:
expect(pub.nextErrLine(), completes);
What is this line?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_with_an_import_error.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_with_an_import_error.dart:33:
endsWith('library handler failed')
How about using a regex matcher for this? Isn't there a matches() function?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/prints_a_transform_error_in_apply.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/prints_a_transform_error_in_apply.dart:14: var
transformer = """
Can you move this to the top-level just so that the lack of indentation doesn't
make the code harder to read?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/prints_a_transform_error_in_apply.dart:44:
expect(server.nextLine(), completion(equals('Build error:')));
Would be great to include the name of the transformer and primary input here.
Maybe do that now, or add a TODO and file a bug?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/runs_a_local_transform_on_the_application_package.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_local_transform_on_the_application_package.dart:30:
requestShouldSucceed("foo.out", "foo.out");
So much work for such a little test. This is awesome!

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/runs_a_transform_on_a_dependency.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_transform_on_a_dependency.dart:13:
integration("runs a local transform on a dependency", () {
Can you add a test that a transformer is not run if the primary asset is in a
package that doesn't reference the transformer?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_transform_on_a_dependency.dart:20:
d.dir("lib", [d.dir("src", [
Can you put the "src" directory on the next line?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:47: /// Returns the `pub serve`
process.
Newline above this.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:609: /// map of dependency names to
paths. [pkg] is a list of Dart SDK packages; each
"Dart SDK packages" -> 'packages in the Dart repo's "pkg" directory'.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:611: /// in the Dart SDK.
"SDK" -> "repo".

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:624:
readTextFile(path.join(packagePath, 'pubspec.yaml')));
How about using Pubspec.parse() here?

[nweiz, 2013-08-28 20:45:23]

I realized while responding that a number of my tests weren't actually being
run, so I enabled them and made a few additional fixes when they weren't
working.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:26: watchSources(graph, barback);
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> I think it's cleaner to do this after the server is bound. Previously, I had
> some race conditions in the tests because notifications could come in before
the
> server had finished starting up, which made the stdout non-deterministic.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:32: // non-programmatic barback
errors, they should take down the whole program.
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "non-programmatic" -> "programmatic"?

No, "non-programmatic" is correct. If e.g. a TransformerException occurs while
loading the transformers, we want to crash; otherwise, the server would be in an
inconsistent state where not all the transformers are loaded.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:72: var idsToPackages = new
Map<AssetId, Set<String>>();
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Document this, maybe:
> 
> // Map each transformer to the names of the packages that use it.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:78: var transformersForId = new
Map<AssetId, Set<Transformer>>();
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Nit: move to just above Future.wait.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:47: return
mirrors.libraries[uri].classes.values.where((mirror) {
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "mirror" -> "classMirror".

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:48: if
(mirror.isPrivate) return false;
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Might want to check and see if the class is abstract too.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:52: if
(!constructor.parameters.isEmpty) return false;
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Add a TODO to report this error to the user.

I don't think this should be an error. Users should be allowed to create
Transformers that can only be instantiated in code, either for use in
meta-transformers or with barback but without the plugin API.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:105: /// Returns
whether or not [superclass] is a superclass of [mirror].
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Clearer would be "[mirror] is an instance of [superclass]."

A class isn't an instance of its superclasses.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:116: Map
_wrapTransformer(Transformer transformer) {
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "wrap" -> "serialize", "unwrap" -> "deserialize" here and elsewhere.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:140: return new
Asset.fromStream(_unwrapId(asset['id']), box.stream);
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> I really think we should serialize assets directly. I would add static
> serialize() and deserialize() methods to Asset that handle the different know
> Asset classes. Then add a TODO to handle unknown asset types.

I'll add a TODO for this.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:144: AssetId
_unwrapId(Map id) => new AssetId(id['package'], id['path']);
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> I'd prefer adding a serialize() method directly on AssetId and an
> AssetId.deserialize() constructor. In both cases, make the data type just
Object
> and not Map.
> 
> Or we could use the serialization package.

I don't like the idea of just manually adding a serialize method to every object
in barback that we plan to send across isolate boundaries here. Currently,
barback doesn't have anything to do with isolates; putting code in there
explicitly for isolate serialization seems like a code smell. The means by which
pub loads transformer plugins is not barback's concern, especially since it's so
easy to manually serialize and deserialize AssetIds.

I don't think the serialization package buys us much. It will work automatically
for a class with a zero-argument constructor where all of its state is
encapsulated in mutable public variables. Otherwise, it needs explicit
instructions about how to serialize and deserialize everything, which is
essentially what we're providing here.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:179: /// Sends the
result of [_sendFuture] from [portCall], which should be the
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "Sends" -> "Receives".

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:256:
_TRANSFORMER_ISOLATE.replaceAll('<<PORT>>', server.port.toString());
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Add a TODO to do something less unholy here...

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:268: /// The port
with which we communicate with the host isolate.
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "host" -> "child"?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:13:
RewriteImportTransformer();
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Remove this?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:34: index
= directive.uri.literal.end;
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Very nice.

Such as it is.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:44: /// A
simple visitor that collects import and export nodes.
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> What about parts?

Ugh. Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/rewrite_import_transformer.dart:48:
_ImportExportCollector();
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Remove this?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/server.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:42: /// This server will serve
assets from [barback], and use [entrypoint] as the
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "entrypoint" -> "rootPackage".

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/server.dart:68:
_methodNotAllowed(request);
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Should add a test for this.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/dart.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:74: /// Returns a Future that will
resolve to a send point that will communicate to
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "send point" -> "[SendPort]".

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:130:
CrossIsolateException.deserialize(Map error)
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Instead of Map here and below, I'd just use Object to completely encapsulate
the
> serialized type.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:192: Future<PackageGraph>
loadPackageGraph() {
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> I wonder if this and lockFile should just be getters. Is there a reason for
them
> not to be now that they are both sync?

This isn't sync. I assumed [loadLockFile] was still a method to indicate that it
did IO and thus was more expensive than a getter usually is, but I wouldn't be
opposed to making it a getter if you thought that was cleaner.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:185: throw new
FormatException('Transformers "$transformers" must be a list.');
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> 'The pubspec field "transformers" must be a list, but was "$transformers".'

I don't like explicitly calling out that this is a pubspec field, since the
error should be automatically be presented in a context where it's clear that
it's coming from a pubspec. I'll chagne the rest of the text, though.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:196: var parts = split1(transformer,
"/");
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Document this chunk of code.
> 
> Also, can you file a bug to document the new pubspec changes on
> pub.dartlang.org?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:202: throw new
FormatException('Transformer "$transformer" not found.');
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Clarify this message, like 'Could not find package for transformer
> "$transformer".'

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/fails_to_load_a_file_that_defines_no_transforms_test.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_file_that_defines_no_transforms_test.dart:21:
d.dir("lib", [d.dir("src", [
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Move d.dir("src") to its own line and indent the next line to make the nesting
a
> bit clearer please. (Or just put transformer.dart in lib.)

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_file_that_defines_no_transforms_test.dart:31:
expect(pub.nextErrLine(), completion(startsWith('required by myapp.')));
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Why not just move these two lines after shouldExit(1) and then do a single
test
> against remainingStderr()?

The assertions make more sense line-by-line than against a whole string.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_from_a_non_dependency.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_from_a_non_dependency.dart:23:
expect(pub.nextErrLine(), completes);
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> What is this line?

It prints the path to the pubspec. We want to ignore it. I'll add a comment.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_with_an_import_error.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/fails_to_load_a_transform_with_an_import_error.dart:33:
endsWith('library handler failed')
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> How about using a regex matcher for this? Isn't there a matches() function?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/prints_a_transform_error_in_apply.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/prints_a_transform_error_in_apply.dart:14: var
transformer = """
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Can you move this to the top-level just so that the lack of indentation
doesn't
> make the code harder to read?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/prints_a_transform_error_in_apply.dart:44:
expect(server.nextLine(), completion(equals('Build error:')));
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Would be great to include the name of the transformer and primary input here.
> Maybe do that now, or add a TODO and file a bug?

It turns out this test wasn't actually getting run, and the real output does
include the transform information. Fixed.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/runs_a_transform_on_a_dependency.dart
(right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_transform_on_a_dependency.dart:13:
integration("runs a local transform on a dependency", () {
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Can you add a test that a transformer is not run if the primary asset is in a
> package that doesn't reference the transformer?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/runs_a_transform_on_a_dependency.dart:20:
d.dir("lib", [d.dir("src", [
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Can you put the "src" directory on the next line?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:47: /// Returns the `pub serve`
process.
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> Newline above this.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:609: /// map of dependency names to
paths. [pkg] is a list of Dart SDK packages; each
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "Dart SDK packages" -> 'packages in the Dart repo's "pkg" directory'.

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:611: /// in the Dart SDK.
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> "SDK" -> "repo".

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:624:
readTextFile(path.join(packagePath, 'pubspec.yaml')));
On 2013/08/27 22:12:30, Bob Nystrom wrote:
> How about using Pubspec.parse() here?

That requires setting up a SourceRegistry, which is more trouble than it's
worth.

[Bob Nystrom, 2013-08-29 00:12:01]

A few more suggestions, otherwise LGTM!

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:105: /// Returns
whether or not [superclass] is a superclass of [mirror].
On 2013/08/28 20:45:23, nweiz wrote:
> On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > Clearer would be "[mirror] is an instance of [superclass]."
> 
> A class isn't an instance of its superclasses.

I guess that depends on your definition of "instance of". How about: "[mirror]
is a subtype of [superclass]"?

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:144: AssetId
_unwrapId(Map id) => new AssetId(id['package'], id['path']);
On 2013/08/28 20:45:23, nweiz wrote:
> On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > I'd prefer adding a serialize() method directly on AssetId and an
> > AssetId.deserialize() constructor. In both cases, make the data type just
> Object
> > and not Map.
> > 
> > Or we could use the serialization package.
> 
> I don't like the idea of just manually adding a serialize method to every
object
> in barback that we plan to send across isolate boundaries here. Currently,
> barback doesn't have anything to do with isolates; putting code in there
> explicitly for isolate serialization seems like a code smell. The means by
which
> pub loads transformer plugins is not barback's concern, especially since it's
so
> easy to manually serialize and deserialize AssetIds.
> 
> I don't think the serialization package buys us much. It will work
automatically
> for a class with a zero-argument constructor where all of its state is
> encapsulated in mutable public variables. Otherwise, it needs explicit
> instructions about how to serialize and deserialize everything, which is
> essentially what we're providing here.

True, but the nice thing is that third-party users could user the serialization
package's mechanics for defining serializers for their types instead of
something barback-specific.

I'm fine with this for now, but we should discuss this more at some point.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:185: throw new
FormatException('Transformers "$transformers" must be a list.');
On 2013/08/28 20:45:23, nweiz wrote:
> On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > 'The pubspec field "transformers" must be a list, but was "$transformers".'
> 
> I don't like explicitly calling out that this is a pubspec field, since the
> error should be automatically be presented in a context where it's clear that
> it's coming from a pubspec. I'll chagne the rest of the text, though.

The other messages in here provide that context explicitly, so we should at
least be consistent.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:624:
readTextFile(path.join(packagePath, 'pubspec.yaml')));
On 2013/08/28 20:45:23, nweiz wrote:
> On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > How about using Pubspec.parse() here?
> 
> That requires setting up a SourceRegistry, which is more trouble than it's
> worth.

You should be able to just make an empty source registry. If a pubspec contains
an unknown source, it just doesn't do any validation on it.

https://codereview.chromium.org/23625002/diff/20001/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/serve/invalid_method_test.dart (right):

https://codereview.chromium.org/23625002/diff/20001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/invalid_method_test.dart:17:
integration("responds with a 404 for missing files", () {
Fix the description.

[nweiz, 2013-09-04 00:21:57]

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:105: /// Returns
whether or not [superclass] is a superclass of [mirror].
On 2013/08/29 00:12:01, Bob Nystrom wrote:
> On 2013/08/28 20:45:23, nweiz wrote:
> > On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > > Clearer would be "[mirror] is an instance of [superclass]."
> > 
> > A class isn't an instance of its superclasses.
> 
> I guess that depends on your definition of "instance of". How about: "[mirror]
> is a subtype of [superclass]"?

Done.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
File sdk/lib/_internal/pub/lib/src/pubspec.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/pubspec.dart:185: throw new
FormatException('Transformers "$transformers" must be a list.');
On 2013/08/29 00:12:01, Bob Nystrom wrote:
> On 2013/08/28 20:45:23, nweiz wrote:
> > On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > > 'The pubspec field "transformers" must be a list, but was
"$transformers".'
> > 
> > I don't like explicitly calling out that this is a pubspec field, since the
> > error should be automatically be presented in a context where it's clear
that
> > it's coming from a pubspec. I'll chagne the rest of the text, though.
> 
> The other messages in here provide that context explicitly, so we should at
> least be consistent.

The existing messages are already inconsistent. I've filed issue 13006 to track
this.

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
File sdk/lib/_internal/pub/test/test_pub.dart (right):

https://codereview.chromium.org/23625002/diff/1/sdk/lib/_internal/pub/test/te...
sdk/lib/_internal/pub/test/test_pub.dart:624:
readTextFile(path.join(packagePath, 'pubspec.yaml')));
On 2013/08/29 00:12:01, Bob Nystrom wrote:
> On 2013/08/28 20:45:23, nweiz wrote:
> > On 2013/08/27 22:12:30, Bob Nystrom wrote:
> > > How about using Pubspec.parse() here?
> > 
> > That requires setting up a SourceRegistry, which is more trouble than it's
> > worth.
> 
> You should be able to just make an empty source registry. If a pubspec
contains
> an unknown source, it just doesn't do any validation on it.

It requires at least a real default source.

https://codereview.chromium.org/23625002/diff/20001/sdk/lib/_internal/pub/tes...
File sdk/lib/_internal/pub/test/serve/invalid_method_test.dart (right):

https://codereview.chromium.org/23625002/diff/20001/sdk/lib/_internal/pub/tes...
sdk/lib/_internal/pub/test/serve/invalid_method_test.dart:17:
integration("responds with a 404 for missing files", () {
On 2013/08/29 00:12:01, Bob Nystrom wrote:
> Fix the description.

Done.

[nweiz, 2013-09-04 00:43:34]

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