Support dart2js in pub serve.

Support dart2js in pub serve.

R=nweiz@google.com

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

[nweiz, 2013-09-27 22:21:17]

It would be nice to have some more error-related tests. Do we give good feedback
if an import fails in dart2js? If there's a syntax error? We should add a TODO
to test that syntax errors point to the correct source locations for transformed
assets once we have better source map support.

Also, test that non-entrypoint libraries in web/ don't get compiled.

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

https://codereview.chromium.org/24886002/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_logger.dart:20: /// the warning.
"warning" -> "message"

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:61: var builtInTransformers = [
Add a TODO here to add a dart2dart transformer and a way of choosing to simulate
production mode.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:154: /// Converts a [uri] served by
pub serve into an [AssetId] that can be
"uri" -> "url", throughout this function (except the type of the parameter).

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:156: AssetId uriToId(String
rootPackage, Uri uri) {
It feels like this should be a method on [BarbackServer]. The server knows its
root package, and it feels weird to have [uri] here without an explicit context
for what server it refers to.

This will also let you assert that the hostname of [uri] (if one exists) matches
the server's.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:165: } on FormatException catch(_) {
Style nit: "catch (_)"

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:168: return null;
It feels like throwing an error for an invalid URI makes sense here.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:176: /// Converts [parts], a list of
path components into an [AssetId] if [parts]
"components" -> "components,"

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:182: AssetId
specialPathToId(List<String> parts) {
As a public API function of this library, it doesn't feel right that this takes
a list of parts. I'd make it just take a relative path instead. An occasional
extra join+split is worth the API cleanliness.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:185: var dirName = pair[1];
Style nit: I like using [Iterable.first] and [Iterable.last] when dealing with a
list that's known to be two elements.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:193: // missing the package name
after it, which makes it invalid here.
Nit: "as a regular path. If it's missing the package name after the special
directory, it's invalid."

I know I wrote this in the first place, but I like the new phrasing better :p.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:195: throw new
FormatException('Expected package named after "$partName".');
"named" -> "name"

I would include more context here. Something like "Invalid package path
$whatever: ...".

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:22: class
Dart2JSTransformer extends Transformer {
This deserves at least a short docstring.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:32:
!asset.id.path.contains("/packages/"));
Pub's barback integration should already ensure that "packages" directories are
kept out of barback.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:44: // relative
paths to.
"gives dart2js a root to resolve relative paths against" (or "against which to
resolve relative paths" if you're me).

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:47:
path.toUri(_graph.packages[id.package].dir).path,
[id.package] will always be the entrypoint, right? We should assert that
somewhere. You could also use [_graph.entrypoint.root.path] here if you want.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:59:
transform.logger.info("Generated $id (${js.length} characters) in
${stopwatch.elapsed}");
Line length.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:66: class
BarbackSourceFileProvider implements SourceFileProvider {
Make this private.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:71: final
sourceFiles = new Map<String, SourceFile>();
Does dart2js automatically memoize using this field? If so, mention that in the
docs.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:75:
Future<String> readStringFromUri(Uri resourceUri) {
It's unclear to me what [resourceUri] is going to look like. Is it always
absolute? If it's relative, what's it relative to?

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:85:
Future<String> call(Uri resourceUri) => readStringFromUri(resourceUri);
What's the difference between [call] and [readStringFromUri]? Document it (if
there isn't a difference, document that that's intentional).

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:105: // example,
with a path dependency. In that case, the path we get from
I don't understand why this would happen with a path dependency -- or if it did,
why it would be specific to path dependencies. As far as dart2js is concerned,
the path to every dependency library looks like
"/path/to/entrypoint/packages/package_name/path/in/package.dart". Only relative
imports from within the entrypoint's "web" directory should be
barback-controlled but not have "packages" in their paths, and even those could
be set apart by adding a flag of some sort to the virtual path we construct for
the entrypoint.

I guess this could also in theory happen if someone did some sort of many-"../"
import, but that's awful and we shouldn't support it if at all possible.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:121: //
SourceFileProvider. Until then, just shut up the warnings.
File an issue for this and reference the number here.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart:13: /// A
single transformer that just forwards any ".dart" file as an output.
Style nit: Paragraph break after the first sentence.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart:15: ///
parallel to make sure the original Dart file is still available.
Mention that this is done to support Dartium.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:26:
Iterable<Transformer> builtInTransformers) {
Make [builtInTransformers] optional.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:24: /// Returns the path to the library
directory. This corresponds to the "sdk"
Paragraph break.

"Library directory" doesn't tell me much, especially given that the name of the
function is [libraryPath]. Something like "the directory containing the Dart
core libraries" would be better.

Is this actually used outside of this file now? If not, it should probably
remain private. At least it should have a different name -- [libraryPath] is
already a function in io.dart.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:36: /// omitted, uses a default
[SourceFileProvider].
"a default [SourceFileProvider] that loads files directly from the filesystem."

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:125: // RegExps are matched,
everything else must be equal. Note that we can't use
At this point, it's probably cleaner to make [expected] be a Matcher (although
untyped as matcher parameters always are). In practice you're usually just
trying to do [contains] anyway.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart
(right):

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart:41:
}
We should run a test like this but with a "package:" import of a generated lib
file as well.

[Bob Nystrom, 2013-09-28 00:56:11]

Thanks!

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

https://codereview.chromium.org/24886002/diff/1/pkg/barback/lib/src/transform...
pkg/barback/lib/src/transform_logger.dart:20: /// the warning.
On 2013/09/27 22:21:17, nweiz wrote:
> "warning" -> "message"

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:61: var builtInTransformers = [
On 2013/09/27 22:21:17, nweiz wrote:
> Add a TODO here to add a dart2dart transformer and a way of choosing to
simulate
> production mode.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:154: /// Converts a [uri] served by
pub serve into an [AssetId] that can be
On 2013/09/27 22:21:17, nweiz wrote:
> "uri" -> "url", throughout this function (except the type of the parameter).

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:156: AssetId uriToId(String
rootPackage, Uri uri) {
On 2013/09/27 22:21:17, nweiz wrote:
> It feels like this should be a method on [BarbackServer]. The server knows its
> root package, and it feels weird to have [uri] here without an explicit
context
> for what server it refers to.

Done.

> This will also let you assert that the hostname of [uri] (if one exists)
matches
> the server's.

Didn't do this. This is called in the request handler for the server, so
checking the hostname feels redundant.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:165: } on FormatException catch(_) {
On 2013/09/27 22:21:17, nweiz wrote:
> Style nit: "catch (_)"

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:168: return null;
On 2013/09/27 22:21:17, nweiz wrote:
> It feels like throwing an error for an invalid URI makes sense here.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:176: /// Converts [parts], a list of
path components into an [AssetId] if [parts]
On 2013/09/27 22:21:17, nweiz wrote:
> "components" -> "components,"

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:182: AssetId
specialPathToId(List<String> parts) {
On 2013/09/27 22:21:17, nweiz wrote:
> As a public API function of this library, it doesn't feel right that this
takes
> a list of parts. I'd make it just take a relative path instead. An occasional
> extra join+split is worth the API cleanliness.

The problem is that this can be called with both URLs and file paths, so it's
ambiguous as to how to properly separate it. So I just rely on the caller to do
that instead.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:185: var dirName = pair[1];
On 2013/09/27 22:21:17, nweiz wrote:
> Style nit: I like using [Iterable.first] and [Iterable.last] when dealing with
a
> list that's known to be two elements.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:193: // missing the package name
after it, which makes it invalid here.
On 2013/09/27 22:21:17, nweiz wrote:
> Nit: "as a regular path. If it's missing the package name after the special
> directory, it's invalid."
> 
> I know I wrote this in the first place, but I like the new phrasing better :p.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:195: throw new
FormatException('Expected package named after "$partName".');
On 2013/09/27 22:21:17, nweiz wrote:
> "named" -> "name"
> 
> I would include more context here. Something like "Invalid package path
> $whatever: ...".

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:22: class
Dart2JSTransformer extends Transformer {
On 2013/09/27 22:21:17, nweiz wrote:
> This deserves at least a short docstring.

Oops. Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:32:
!asset.id.path.contains("/packages/"));
On 2013/09/27 22:21:17, nweiz wrote:
> Pub's barback integration should already ensure that "packages" directories
are
> kept out of barback.

It wasn't. Done now.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:44: // relative
paths to.
On 2013/09/27 22:21:17, nweiz wrote:
> "gives dart2js a root to resolve relative paths against" (or "against which to
> resolve relative paths" if you're me).

Dangling prepositions are perfectly grammatical! :)

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:47:
path.toUri(_graph.packages[id.package].dir).path,
On 2013/09/27 22:21:17, nweiz wrote:
> [id.package] will always be the entrypoint, right? We should assert that
> somewhere. You could also use [_graph.entrypoint.root.path] here if you want.

It will always be the entrypoint, but this is the *dart* entrypoint, the file
that has main(), not the entrypoint *package*. Our terminology is confusing
here. Maybe we should use "root" for packages. :-/

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:59:
transform.logger.info("Generated $id (${js.length} characters) in
${stopwatch.elapsed}");
On 2013/09/27 22:21:17, nweiz wrote:
> Line length.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:66: class
BarbackSourceFileProvider implements SourceFileProvider {
On 2013/09/27 22:21:17, nweiz wrote:
> Make this private.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:71: final
sourceFiles = new Map<String, SourceFile>();
On 2013/09/27 22:21:17, nweiz wrote:
> Does dart2js automatically memoize using this field? If so, mention that in
the
> docs.

It does. I really wish this class separated out the things an external library
needs to implement from the random set of things its built in source provider
implements and dart2js uses internally.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:75:
Future<String> readStringFromUri(Uri resourceUri) {
On 2013/09/27 22:21:17, nweiz wrote:
> It's unclear to me what [resourceUri] is going to look like. Is it always
> absolute? If it's relative, what's it relative to?

From what I've seen, it's always absolute. It looks like it is the "file:" URI
to a given Dart file after relative and "package:" URIs have been resolved.

Added some comments/asserts. As an aside, dart2js apparently doesn't support
"http:" imports.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:85:
Future<String> call(Uri resourceUri) => readStringFromUri(resourceUri);
On 2013/09/27 22:21:17, nweiz wrote:
> What's the difference between [call] and [readStringFromUri]? Document it (if
> there isn't a difference, document that that's intentional).

No idea. Dart2js has this in its interface, so I'm just following along. I think
"SoureFileProvider" used to just be a function and it was changed to an
interface at some point. This may be a vestigal organ from those days.

Added a comment.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:105: // example,
with a path dependency. In that case, the path we get from
On 2013/09/27 22:21:17, nweiz wrote:
> I don't understand why this would happen with a path dependency -- or if it
did,
> why it would be specific to path dependencies. As far as dart2js is concerned,
> the path to every dependency library looks like
> "/path/to/entrypoint/packages/package_name/path/in/package.dart". Only
relative
> imports from within the entrypoint's "web" directory should be
> barback-controlled but not have "packages" in their paths, and even those
could
> be set apart by adding a flag of some sort to the virtual path we construct
for
> the entrypoint.
> 
> I guess this could also in theory happen if someone did some sort of
many-"../"
> import, but that's awful and we shouldn't support it if at all possible.

I thought I ran into a case while manually testing where this is needed, but I
can't repro it. We do need to test if it's under the root package, but I'm not
sure about others anymore. I'll take this out.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:121: //
SourceFileProvider. Until then, just shut up the warnings.
On 2013/09/27 22:21:17, nweiz wrote:
> File an issue for this and reference the number here.

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart:13: /// A
single transformer that just forwards any ".dart" file as an output.
On 2013/09/27 22:21:17, nweiz wrote:
> Style nit: Paragraph break after the first sentence.

Done.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart_forwarding_transformer.dart:15: ///
parallel to make sure the original Dart file is still available.
On 2013/09/27 22:21:17, nweiz wrote:
> Mention that this is done to support Dartium.

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:26:
Iterable<Transformer> builtInTransformers) {
On 2013/09/27 22:21:17, nweiz wrote:
> Make [builtInTransformers] optional.

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:24: /// Returns the path to the library
directory. This corresponds to the "sdk"
On 2013/09/27 22:21:17, nweiz wrote:
> Paragraph break.

Done.

> 
> "Library directory" doesn't tell me much, especially given that the name of
the
> function is [libraryPath]. Something like "the directory containing the Dart
> core libraries" would be better.

Done.

> 
> Is this actually used outside of this file now? If not, it should probably
> remain private. At least it should have a different name -- [libraryPath] is
> already a function in io.dart.

Nope. I was using it but ended up doing a different approach. Made it private
again.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/dart.dart:36: /// omitted, uses a default
[SourceFileProvider].
On 2013/09/27 22:21:17, nweiz wrote:
> "a default [SourceFileProvider] that loads files directly from the
filesystem."

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:125: // RegExps are matched,
everything else must be equal. Note that we can't use
On 2013/09/27 22:21:17, nweiz wrote:
> At this point, it's probably cleaner to make [expected] be a Matcher (although
> untyped as matcher parameters always are). In practice you're usually just
> trying to do [contains] anyway.

Most of the tests are testing for exact quality, and I don't want to add
equals() to every call to this, so I think this leads to the simplest code at
the callsite.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart
(right):

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart:41:
}
On 2013/09/27 22:21:17, nweiz wrote:
> We should run a test like this but with a "package:" import of a generated lib
> file as well.

I figured that was overkill. We already have tests for importing generated
stuff, and there isn't anything special about dart2js consuming it.

[nweiz, 2013-09-30 17:33:34]

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:182: AssetId
specialPathToId(List<String> parts) {
On 2013/09/28 00:56:11, Bob Nystrom wrote:
> On 2013/09/27 22:21:17, nweiz wrote:
> > As a public API function of this library, it doesn't feel right that this
> takes
> > a list of parts. I'd make it just take a relative path instead. An
occasional
> > extra join+split is worth the API cleanliness.
> 
> The problem is that this can be called with both URLs and file paths, so it's
> ambiguous as to how to properly separate it. So I just rely on the caller to
do
> that instead.

That's easy to change by delaying the URI-to-path conversion of the
[resourceUri] argument to [BarbackSourceFileProvider.readStringFromUri]. If you
leave it as a URI when it's passed to [_sourcePathToId], you can pass in the
URI's path component to this function.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:47:
path.toUri(_graph.packages[id.package].dir).path,
On 2013/09/28 00:56:11, Bob Nystrom wrote:
> On 2013/09/27 22:21:17, nweiz wrote:
> > [id.package] will always be the entrypoint, right? We should assert that
> > somewhere. You could also use [_graph.entrypoint.root.path] here if you
want.
> 
> It will always be the entrypoint, but this is the *dart* entrypoint, the file
> that has main(), not the entrypoint *package*. Our terminology is confusing
> here. Maybe we should use "root" for packages. :-/

Right, I get that; what I'm saying is that [id.package] here will always refer
to the entrypoint package, since this transformer will only ever be run on the
entrypoint package. Given that, it seems weird to call
`_graph.packages[id.package]` when it will always return the same thing as
`_graph.entrypoint.root`.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:125: // RegExps are matched,
everything else must be equal. Note that we can't use
On 2013/09/28 00:56:11, Bob Nystrom wrote:
> On 2013/09/27 22:21:17, nweiz wrote:
> > At this point, it's probably cleaner to make [expected] be a Matcher
(although
> > untyped as matcher parameters always are). In practice you're usually just
> > trying to do [contains] anyway.
> 
> Most of the tests are testing for exact quality, and I don't want to add
> equals() to every call to this, so I think this leads to the simplest code at
> the callsite.

Passing in a string will implicitly call [equals]; I'd be fine with that here.
As it stands the call-site for non-equality checks is much messier than it would
be otherwise.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart
(right):

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart:41:
}
On 2013/09/28 00:56:11, Bob Nystrom wrote:
> On 2013/09/27 22:21:17, nweiz wrote:
> > We should run a test like this but with a "package:" import of a generated
lib
> > file as well.
> 
> I figured that was overkill. We already have tests for importing generated
> stuff, and there isn't anything special about dart2js consuming it.

There's enough machinery going on with how the dart2js integration resolves
paths that I think it's worth testing both relative and package: imports.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:30: //
TODO(rnystrom): Should we see if the file is actually an entrypoint too?
We should check this in [apply], not [isPrimary]. But it's definitely something
we should do; otherwise any random dart file in web/ will cause a build failure.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:78: /// the same
file multiple times.
Paragraph break.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:118: var rootDir
= _graph.entrypoint.root.dir;
This should be "path.join(_graph.entrypoint.root.dir, 'web')". Only web files
will contain relative imports outside of a "packages/" directory; even lib files
in the entrypoint will themselves be imported using "package:".

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:118: if
(builtInTransformers.isNotEmpty) {
Since this is now optional, check if it's null as well.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/watch_sources.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/watch_sources.dart:30: // TODO(rnystrom):
If pub gets rid of symlinks, remove this.
I'm not sure this TODO is correct. Even if we don't create symlinks anymore,
anything in a "packages" directory will still ultimately be invisible to the
server since it treats "packages/" URIs specially (same for "assets").

[Bob Nystrom, 2013-10-01 19:08:55]

Thanks!

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback.dart:182: AssetId
specialPathToId(List<String> parts) {
On 2013/09/30 17:33:34, nweiz wrote:
> On 2013/09/28 00:56:11, Bob Nystrom wrote:
> > On 2013/09/27 22:21:17, nweiz wrote:
> > > As a public API function of this library, it doesn't feel right that this
> > takes
> > > a list of parts. I'd make it just take a relative path instead. An
> occasional
> > > extra join+split is worth the API cleanliness.
> > 
> > The problem is that this can be called with both URLs and file paths, so
it's
> > ambiguous as to how to properly separate it. So I just rely on the caller to
> do
> > that instead.
> 
> That's easy to change by delaying the URI-to-path conversion of the
> [resourceUri] argument to [BarbackSourceFileProvider.readStringFromUri]. If
you
> leave it as a URI when it's passed to [_sourcePathToId], you can pass in the
> URI's path component to this function.

Done.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:47:
path.toUri(_graph.packages[id.package].dir).path,
On 2013/09/30 17:33:34, nweiz wrote:
> On 2013/09/28 00:56:11, Bob Nystrom wrote:
> > On 2013/09/27 22:21:17, nweiz wrote:
> > > [id.package] will always be the entrypoint, right? We should assert that
> > > somewhere. You could also use [_graph.entrypoint.root.path] here if you
> want.
> > 
> > It will always be the entrypoint, but this is the *dart* entrypoint, the
file
> > that has main(), not the entrypoint *package*. Our terminology is confusing
> > here. Maybe we should use "root" for packages. :-/
> 
> Right, I get that; what I'm saying is that [id.package] here will always refer
> to the entrypoint package, since this transformer will only ever be run on the
> entrypoint package. Given that, it seems weird to call
> `_graph.packages[id.package]` when it will always return the same thing as
> `_graph.entrypoint.root`.

Ah, I see what you're getting at.

It is true that because isPrimary returns false on any path that doesn't start
with "web" and the only package whose "web" directory is included is the root
package. Because of that, apply() will only be called with primaries in the root
package.

But it feels weird to me to rely on (and assert) that fact here when making the
code more general doesn't make it any more verbose. If, for some reason, we
change this transformer so that you can run dart2js on other dart files outside
of "web" (imagine you want to use a separately compiled component from some
other package or something), I like that this piece of code will already work
then.

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

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/utils.dart:125: // RegExps are matched,
everything else must be equal. Note that we can't use
On 2013/09/30 17:33:34, nweiz wrote:
> On 2013/09/28 00:56:11, Bob Nystrom wrote:
> > On 2013/09/27 22:21:17, nweiz wrote:
> > > At this point, it's probably cleaner to make [expected] be a Matcher
> (although
> > > untyped as matcher parameters always are). In practice you're usually just
> > > trying to do [contains] anyway.
> > 
> > Most of the tests are testing for exact quality, and I don't want to add
> > equals() to every call to this, so I think this leads to the simplest code
at
> > the callsite.
> 
> Passing in a string will implicitly call [equals]; I'd be fine with that here.
> As it stands the call-site for non-equality checks is much messier than it
would
> be otherwise.

OK, done. I didn't do that before because I thought you weren't keen on relying
on the implicit equals() of non-Matcher arguments. Agreed that using contains()
looks better than a RegExp.

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
File
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart
(right):

https://codereview.chromium.org/24886002/diff/1/sdk/lib/_internal/pub/test/tr...
sdk/lib/_internal/pub/test/transformer/dart2js/compiles_imported_generated_file_test.dart:41:
}
On 2013/09/30 17:33:34, nweiz wrote:
> On 2013/09/28 00:56:11, Bob Nystrom wrote:
> > On 2013/09/27 22:21:17, nweiz wrote:
> > > We should run a test like this but with a "package:" import of a generated
> lib
> > > file as well.
> > 
> > I figured that was overkill. We already have tests for importing generated
> > stuff, and there isn't anything special about dart2js consuming it.
> 
> There's enough machinery going on with how the dart2js integration resolves
> paths that I think it's worth testing both relative and package: imports.

Done.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:30: //
TODO(rnystrom): Should we see if the file is actually an entrypoint too?
On 2013/09/30 17:33:34, nweiz wrote:
> We should check this in [apply], not [isPrimary]. But it's definitely
something
> we should do; otherwise any random dart file in web/ will cause a build
failure.

Done.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:78: /// the same
file multiple times.
On 2013/09/30 17:33:34, nweiz wrote:
> Paragraph break.

Done.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:118: var rootDir
= _graph.entrypoint.root.dir;
On 2013/09/30 17:33:34, nweiz wrote:
> This should be "path.join(_graph.entrypoint.root.dir, 'web')". Only web files
> will contain relative imports outside of a "packages/" directory; even lib
files
> in the entrypoint will themselves be imported using "package:".

The relative path in the AssetID needs to be relative to the package root
directory, not "web". I could do path.join(..., "web") just in the isBeneath()
call, but I don't know if that adds any value. There's nothing officially
prohibiting you from doing import "../foo/bar.dart" from inside your "web"
directory.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/load_all_transformers.dart:118: if
(builtInTransformers.isNotEmpty) {
On 2013/09/30 17:33:34, nweiz wrote:
> Since this is now optional, check if it's null as well.

Done.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/watch_sources.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/watch_sources.dart:30: // TODO(rnystrom):
If pub gets rid of symlinks, remove this.
On 2013/09/30 17:33:34, nweiz wrote:
> I'm not sure this TODO is correct. Even if we don't create symlinks anymore,
> anything in a "packages" directory will still ultimately be invisible to the
> server since it treats "packages/" URIs specially (same for "assets").

True, but if we don't create symlinks anymore, we shouldn't even have "packages"
directories in there at all. I suppose a user could manually create one, but I'm
not worried about that.

[nweiz, 2013-10-01 20:46:54]

A couple more suggestions, then lgtm.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:30: //
TODO(rnystrom): Should we see if the file is actually an entrypoint too?
On 2013/10/01 19:08:55, Bob Nystrom wrote:
> On 2013/09/30 17:33:34, nweiz wrote:
> > We should check this in [apply], not [isPrimary]. But it's definitely
> something
> > we should do; otherwise any random dart file in web/ will cause a build
> failure.
> 
> Done.

Now you can remove the TODO :).

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:118: var rootDir
= _graph.entrypoint.root.dir;
On 2013/10/01 19:08:55, Bob Nystrom wrote:
> On 2013/09/30 17:33:34, nweiz wrote:
> > This should be "path.join(_graph.entrypoint.root.dir, 'web')". Only web
files
> > will contain relative imports outside of a "packages/" directory; even lib
> files
> > in the entrypoint will themselves be imported using "package:".
> 
> The relative path in the AssetID needs to be relative to the package root
> directory, not "web". I could do path.join(..., "web") just in the isBeneath()
> call, but I don't know if that adds any value. There's nothing officially
> prohibiting you from doing import "../foo/bar.dart" from inside your "web"
> directory.

That sort of import is incompatible with "package:" imports of the same file, so
it's already effectively prohibited. It would probably be best if we threw an
exception here, actually, rather than trying to return any file.

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

https://codereview.chromium.org/24886002/diff/25001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback.dart:161: /// throws a [FormatException].
Update the doc comment to reflect the new signature.

https://codereview.chromium.org/24886002/diff/25001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/24886002/diff/25001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:46:
transform.logger.warning(e.message);
Why is this a warning, as opposed to an error? If dart2js found a syntax error,
that would be reported as an error; why are syntax errors detected by the
analyzer different?

[Bob Nystrom, 2013-10-03 17:36:24]

Thanks!

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:30: //
TODO(rnystrom): Should we see if the file is actually an entrypoint too?
On 2013/10/01 20:46:54, nweiz wrote:
> On 2013/10/01 19:08:55, Bob Nystrom wrote:
> > On 2013/09/30 17:33:34, nweiz wrote:
> > > We should check this in [apply], not [isPrimary]. But it's definitely
> > something
> > > we should do; otherwise any random dart file in web/ will cause a build
> > failure.
> > 
> > Done.
> 
> Now you can remove the TODO :).

Done.

https://codereview.chromium.org/24886002/diff/11001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:118: var rootDir
= _graph.entrypoint.root.dir;
On 2013/10/01 20:46:54, nweiz wrote:
> On 2013/10/01 19:08:55, Bob Nystrom wrote:
> > On 2013/09/30 17:33:34, nweiz wrote:
> > > This should be "path.join(_graph.entrypoint.root.dir, 'web')". Only web
> files
> > > will contain relative imports outside of a "packages/" directory; even lib
> > files
> > > in the entrypoint will themselves be imported using "package:".
> > 
> > The relative path in the AssetID needs to be relative to the package root
> > directory, not "web". I could do path.join(..., "web") just in the
isBeneath()
> > call, but I don't know if that adds any value. There's nothing officially
> > prohibiting you from doing import "../foo/bar.dart" from inside your "web"
> > directory.
> 
> That sort of import is incompatible with "package:" imports of the same file,
so
> it's already effectively prohibited.

If your package is an app package, you may not need to support "package:"
imports anyway. I agree that this is a bad thing for users to do, but I'm not
sure if this is the best place for policy-level code like that. For now, I think
I'll leave this code simple, but I'm definitely up for validating that at some
point somewhere in pub serve.

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

https://codereview.chromium.org/24886002/diff/25001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback.dart:161: /// throws a [FormatException].
On 2013/10/01 20:46:54, nweiz wrote:
> Update the doc comment to reflect the new signature.

Done.

https://codereview.chromium.org/24886002/diff/25001/sdk/lib/_internal/pub/lib...
File sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart (right):

https://codereview.chromium.org/24886002/diff/25001/sdk/lib/_internal/pub/lib...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:46:
transform.logger.warning(e.message);
On 2013/10/01 20:46:54, nweiz wrote:
> Why is this a warning, as opposed to an error? If dart2js found a syntax
error,
> that would be reported as an error; why are syntax errors detected by the
> analyzer different?

The same code in pub deploy was using a warning so I did the same here. You're
right, though. Changed it to an error.

[Bob Nystrom, 2013-10-03 17:37:23]

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