Support Dart core libraries in source maps.

Support Dart core libraries in source maps.

BUG=https://code.google.com/p/dart/issues/detail?id=17460
RELNOTE=Support Dart core libraries in source maps in pub build and serve.
R=nweiz@google.com

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

[nweiz, 2014-03-21 19:36:05]

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:348: .where((file)
=> path.extension(file) == ".dart")
Why exclude non-Dart files? I could imagine a transformer wanting to read e.g.
JavaScript from the SDK.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:350:
path.join("lib", path.relative(file, from: sdk.rootDirectory))));
Since we're listing files beneath sdk/lib/ and making [file] relative to sdk/,
won't the results of [path.relative] start with lib/ already? If so, why are we
adding lib/ to the beginning?

Okay I just read the explanation of this in PubPackageProvider. It still seems
very weird to me. I think users will be very confused if they have to write
"lib/lib/..." to load the SDK sources as well.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:232: //
pseudo-package so that it puts correct relative URLs in the source maps.
"in the output directory (which is inside the $sdk pseudo-package) so that"

"output directory" is also kind of confusing, because no such thing exists when
using "pub serve".

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:241:
entrypointPath[0], "packages", r"$sdk");
Won't this break if the user does e.g. "pub build example/foo"?
`entrypointPath[0]` will be "example", but the packages/ directory will be under
build/example/foo/.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:356: // absolute
file paths in imports that don't touch a package.
I think we should stop allowing imports from the filesystem. No other
transformer will support it.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/pub_package_provider.dart:46: var
assetPath = path.relative(path.fromUri(id.path), from: "lib");
This [path.relative] call is confusing, since we're not really computing the
path relative to the lib/ directory in the CWD. I think it would be clearer to
write:

    var components = path.split(path.fromUri(id.path));
    assert(components[0] == 'lib');
    components[0] = sdk.rootDirectory;
    return new Future.value(new Asset.fromPath(id, path.joinAll(components)));

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:30: /// The URI to the root directory
where "dart:" libraries can be found. This
Single-sentence first paragraph.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/dart2js/source_maps_include_core_libs_test.dart
(right):

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/source_maps_include_core_libs_test.dart:17:
// (StringBuffer) and validate that it's source ends up in the source map.
"it's" -> "its"

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/source_maps_include_core_libs_test.dart:56:
}
Also test "pub build --mode debug web/sub".

[Bob Nystrom, 2014-03-24 20:45:56]

Thanks!

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:348: .where((file)
=> path.extension(file) == ".dart")
On 2014/03/21 19:36:06, nweiz wrote:
> Why exclude non-Dart files? I could imagine a transformer wanting to read e.g.
> JavaScript from the SDK.

It ensures that we don't end up with anything left in a release build. I'm OK
with possibly including other files too, but I'd like to see a customer
specifically request it before we make everyone's build output include this
stuff.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:350:
path.join("lib", path.relative(file, from: sdk.rootDirectory))));
On 2014/03/21 19:36:06, nweiz wrote:
> Since we're listing files beneath sdk/lib/ and making [file] relative to sdk/,
> won't the results of [path.relative] start with lib/ already? If so, why are
we
> adding lib/ to the beginning?
> 
> Okay I just read the explanation of this in PubPackageProvider. It still seems
> very weird to me. I think users will be very confused if they have to write
> "lib/lib/..." to load the SDK sources as well.

Yeah, the double lib is annoying, but I don't think there's much I can do about
it.

We need to give dart2js a base URL that it uses for libraries, and that's
"packages/$sdk". It then adds "lib" itself (inside the bowels of dart2js), so we
get a path like "packages/$sdk/lib" which becomes the asset path "$sdk|lib/lib".

> I think users will be very confused if they have to write
"lib/lib/..." to load the SDK sources as well.

I know. :(

We should probably have an API for working with "$sdk" that hides that. In the
meantime, the people using it are pretty sophisticated and can tolerate a little
ugliness.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:232: //
pseudo-package so that it puts correct relative URLs in the source maps.
On 2014/03/21 19:36:06, nweiz wrote:
> "in the output directory (which is inside the $sdk pseudo-package) so that"
> 
> "output directory" is also kind of confusing, because no such thing exists
when
> using "pub serve".

Rewrote. Going forward, I think "build directory" should be the canonical term
for "directory in entrypoint package added to set of sources for pub serve and
build".

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:241:
entrypointPath[0], "packages", r"$sdk");
On 2014/03/21 19:36:06, nweiz wrote:
> Won't this break if the user does e.g. "pub build example/foo"?
> `entrypointPath[0]` will be "example", but the packages/ directory will be
under
> build/example/foo/.

Yup. Fixed.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:356: // absolute
file paths in imports that don't touch a package.
On 2014/03/21 19:36:06, nweiz wrote:
> I think we should stop allowing imports from the filesystem. No other
> transformer will support it.

I'm certainly not crazy about this, but if a user has a program that contains
something like:

import '/Users/me/some/file.dart';

The program isn't incorrect. It will run in the VM and will run if you invoke
dart2js explicitly. In fact, I occasionally do this in hack temp code.

If it doesn't work in pub, they will file bugs.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/pub_package_provider.dart:46: var
assetPath = path.relative(path.fromUri(id.path), from: "lib");
On 2014/03/21 19:36:06, nweiz wrote:
> This [path.relative] call is confusing, since we're not really computing the
> path relative to the lib/ directory in the CWD. I think it would be clearer to
> write:
> 
>     var components = path.split(path.fromUri(id.path));
>     assert(components[0] == 'lib');
>     components[0] = sdk.rootDirectory;
>     return new Future.value(new Asset.fromPath(id, path.joinAll(components)));

Done, more or less.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/dart.dart:30: /// The URI to the root directory
where "dart:" libraries can be found. This
On 2014/03/21 19:36:06, nweiz wrote:
> Single-sentence first paragraph.

Done.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/test/t...
File
sdk/lib/_internal/pub/test/transformer/dart2js/source_maps_include_core_libs_test.dart
(right):

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/source_maps_include_core_libs_test.dart:17:
// (StringBuffer) and validate that it's source ends up in the source map.
On 2014/03/21 19:36:06, nweiz wrote:
> "it's" -> "its"

Done.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/test/t...
sdk/lib/_internal/pub/test/transformer/dart2js/source_maps_include_core_libs_test.dart:56:
}
On 2014/03/21 19:36:06, nweiz wrote:
> Also test "pub build --mode debug web/sub".

Pub build doesn't support subdirectories right now, but added a test that uses
pub serve on a subdirectory (which is good because it also means we're testing
this in pub serve too now).

[nweiz, 2014-03-24 21:53:47]

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:350:
path.join("lib", path.relative(file, from: sdk.rootDirectory))));
On 2014/03/24 20:45:58, Bob Nystrom wrote:
> On 2014/03/21 19:36:06, nweiz wrote:
> > Since we're listing files beneath sdk/lib/ and making [file] relative to
sdk/,
> > won't the results of [path.relative] start with lib/ already? If so, why are
> we
> > adding lib/ to the beginning?
> > 
> > Okay I just read the explanation of this in PubPackageProvider. It still
seems
> > very weird to me. I think users will be very confused if they have to write
> > "lib/lib/..." to load the SDK sources as well.
> 
> Yeah, the double lib is annoying, but I don't think there's much I can do
about
> it.
> 
> We need to give dart2js a base URL that it uses for libraries, and that's
> "packages/$sdk". It then adds "lib" itself (inside the bowels of dart2js), so
we
> get a path like "packages/$sdk/lib" which becomes the asset path
"$sdk|lib/lib".

Mention that in the comment explaining this. Also maybe file a bug to remove
this hard-coding from dart2js.

> > I think users will be very confused if they have to write
> "lib/lib/..." to load the SDK sources as well.
> 
> I know. :(
> 
> We should probably have an API for working with "$sdk" that hides that. In the
> meantime, the people using it are pretty sophisticated and can tolerate a
little
> ugliness.

I'd rather just document "these are some special assets" and have fewer layers
between users and what's going on, but if we can't get dart2js to change then I
guess we'll have to.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:232: //
pseudo-package so that it puts correct relative URLs in the source maps.
On 2014/03/24 20:45:58, Bob Nystrom wrote:
> On 2014/03/21 19:36:06, nweiz wrote:
> > "in the output directory (which is inside the $sdk pseudo-package) so that"
> > 
> > "output directory" is also kind of confusing, because no such thing exists
> when
> > using "pub serve".
> 
> Rewrote. Going forward, I think "build directory" should be the canonical term
> for "directory in entrypoint package added to set of sources for pub serve and
> build".

"Build directory" seems pretty confusing since there's a directory involved in
this process literally named "build".

Whatever terminology you choose, you should update the documentation on
[BuildEnvironment._directories] to reflect it.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:356: // absolute
file paths in imports that don't touch a package.
On 2014/03/24 20:45:58, Bob Nystrom wrote:
> On 2014/03/21 19:36:06, nweiz wrote:
> > I think we should stop allowing imports from the filesystem. No other
> > transformer will support it.
> 
> I'm certainly not crazy about this, but if a user has a program that contains
> something like:
> 
> import '/Users/me/some/file.dart';
> 
> The program isn't incorrect. It will run in the VM and will run if you invoke
> dart2js explicitly. In fact, I occasionally do this in hack temp code.
> 
> If it doesn't work in pub, they will file bugs.

It will break in confusing ways in Dartium or in the presence of other
transformers. We should fail consistently and as quickly as possible if they do
something as unportable as this.

Besides, if users are writing quick hacky code, they won't be running it through
dart2js, they'll be running it in Dartium, not dart2js.

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

https://codereview.chromium.org/207423003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:234: // relative
to the root build directory containing that entrypoint.
It looks like here you're using "build directory" to refer to an output
directory rather than a source directory?

https://codereview.chromium.org/207423003/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:238: //
"web/sub". This means the SDK sources will be (conceptually) at:
Rather than "will be (conceptually)", maybe be more explicit and say something
like "dart2js will believe the SDK sources are at"?

[Bob Nystrom, 2014-03-24 22:52:19]

Thanks!

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:350:
path.join("lib", path.relative(file, from: sdk.rootDirectory))));
On 2014/03/24 21:53:47, nweiz wrote:
> On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > On 2014/03/21 19:36:06, nweiz wrote:
> > > Since we're listing files beneath sdk/lib/ and making [file] relative to
> sdk/,
> > > won't the results of [path.relative] start with lib/ already? If so, why
are
> > we
> > > adding lib/ to the beginning?
> > > 
> > > Okay I just read the explanation of this in PubPackageProvider. It still
> seems
> > > very weird to me. I think users will be very confused if they have to
write
> > > "lib/lib/..." to load the SDK sources as well.
> > 
> > Yeah, the double lib is annoying, but I don't think there's much I can do
> about
> > it.
> > 
> > We need to give dart2js a base URL that it uses for libraries, and that's
> > "packages/$sdk". It then adds "lib" itself (inside the bowels of dart2js),
so
> we
> > get a path like "packages/$sdk/lib" which becomes the asset path
> "$sdk|lib/lib".
> 
> Mention that in the comment explaining this. Also maybe file a bug to remove
> this hard-coding from dart2js.

Filed a bug. Mentioned in build_environment.dart.

> 
> > > I think users will be very confused if they have to write
> > "lib/lib/..." to load the SDK sources as well.
> > 
> > I know. :(
> > 
> > We should probably have an API for working with "$sdk" that hides that. In
the
> > meantime, the people using it are pretty sophisticated and can tolerate a
> little
> > ugliness.
> 
> I'd rather just document "these are some special assets" and have fewer layers
> between users and what's going on, but if we can't get dart2js to change then
I
> guess we'll have to.

Works for me.

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:232: //
pseudo-package so that it puts correct relative URLs in the source maps.
On 2014/03/24 21:53:47, nweiz wrote:
> On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > On 2014/03/21 19:36:06, nweiz wrote:
> > > "in the output directory (which is inside the $sdk pseudo-package) so
that"
> > > 
> > > "output directory" is also kind of confusing, because no such thing exists
> > when
> > > using "pub serve".
> > 
> > Rewrote. Going forward, I think "build directory" should be the canonical
term
> > for "directory in entrypoint package added to set of sources for pub serve
and
> > build".
> 
> "Build directory" seems pretty confusing since there's a directory involved in
> this process literally named "build".

Yeah, it was the best I could come up with, and mirrors "build environment". I
suppose "transformer environment" would be more precise, but that doesn't
exactly roll off the tongue. Reworded the docs a bit here to clarify.

> Whatever terminology you choose, you should update the documentation on
> [BuildEnvironment._directories] to reflect it.

It says:

/// The public directories in the root package that are available for
/// building, keyed by their root directory.

which sounds about right to me.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:356: // absolute
file paths in imports that don't touch a package.
On 2014/03/24 21:53:47, nweiz wrote:
> On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > On 2014/03/21 19:36:06, nweiz wrote:
> > > I think we should stop allowing imports from the filesystem. No other
> > > transformer will support it.
> > 
> > I'm certainly not crazy about this, but if a user has a program that
contains
> > something like:
> > 
> > import '/Users/me/some/file.dart';
> > 
> > The program isn't incorrect. It will run in the VM and will run if you
invoke
> > dart2js explicitly. In fact, I occasionally do this in hack temp code.
> > 
> > If it doesn't work in pub, they will file bugs.
> 
> It will break in confusing ways in Dartium or in the presence of other
> transformers. We should fail consistently and as quickly as possible if they
do
> something as unportable as this.

I'm sympathetic to this, but my experience with pub build has been that I keep
underestimating the things people want to do with it. I'd like to help them
avoid shooting themselves in the foot here. But at the same time, when the user
*does* know what they're doing, it's a drag if pub pretends to know better than
they do and prohibits it.

> Besides, if users are writing quick hacky code, they won't be running it
through
> dart2js, they'll be running it in Dartium, not dart2js.

One thing I've learned since we launched the new pub build is that users use it
quite frequently, even while debugging or otherwise playing around.

How about I leave this in but add a warning when it gets hit?

[nweiz, 2014-03-24 23:17:14]

lgtm

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:232: //
pseudo-package so that it puts correct relative URLs in the source maps.
On 2014/03/24 22:52:19, Bob Nystrom wrote:
> On 2014/03/24 21:53:47, nweiz wrote:
> > On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > > On 2014/03/21 19:36:06, nweiz wrote:
> > > > "in the output directory (which is inside the $sdk pseudo-package) so
> that"
> > > > 
> > > > "output directory" is also kind of confusing, because no such thing
exists
> > > when
> > > > using "pub serve".
> > > 
> > > Rewrote. Going forward, I think "build directory" should be the canonical
> term
> > > for "directory in entrypoint package added to set of sources for pub serve
> and
> > > build".
> > 
> > "Build directory" seems pretty confusing since there's a directory involved
in
> > this process literally named "build".
> 
> Yeah, it was the best I could come up with, and mirrors "build environment". I
> suppose "transformer environment" would be more precise, but that doesn't
> exactly roll off the tongue. Reworded the docs a bit here to clarify.

What about "source directory"? It's up to you, but I really found "build
directory" confusing when reading the associated documentation.

> > Whatever terminology you choose, you should update the documentation on
> > [BuildEnvironment._directories] to reflect it.
> 
> It says:
> 
> /// The public directories in the root package that are available for
> /// building, keyed by their root directory.
> 
> which sounds about right to me. 

My point is less that the existing documentation is unclear, more that if you're
defining a specific term that would be a nice place to introduce it.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:356: // absolute
file paths in imports that don't touch a package.
On 2014/03/24 22:52:19, Bob Nystrom wrote:
> On 2014/03/24 21:53:47, nweiz wrote:
> > On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > > On 2014/03/21 19:36:06, nweiz wrote:
> > > > I think we should stop allowing imports from the filesystem. No other
> > > > transformer will support it.
> > > 
> > > I'm certainly not crazy about this, but if a user has a program that
> contains
> > > something like:
> > > 
> > > import '/Users/me/some/file.dart';
> > > 
> > > The program isn't incorrect. It will run in the VM and will run if you
> invoke
> > > dart2js explicitly. In fact, I occasionally do this in hack temp code.
> > > 
> > > If it doesn't work in pub, they will file bugs.
> > 
> > It will break in confusing ways in Dartium or in the presence of other
> > transformers. We should fail consistently and as quickly as possible if they
> do
> > something as unportable as this.
> 
> I'm sympathetic to this, but my experience with pub build has been that I keep
> underestimating the things people want to do with it. I'd like to help them
> avoid shooting themselves in the foot here. But at the same time, when the
user
> *does* know what they're doing, it's a drag if pub pretends to know better
than
> they do and prohibits it.

The thing is, it's going to be prohibited one way or another almost all the
time. If we add the analyzer as a linting transformer, it will break. If the
user starts using Polymer, it will break. If the user starts using Dartium, it
will break. If the user shares the package, it will break.

I'm also worried about users starting to use this and creating a culture where
all transformer authors feel obligated to add special-casing for filesystem
paths in order to be compatible with existing Dart code.

> > Besides, if users are writing quick hacky code, they won't be running it
> through
> > dart2js, they'll be running it in Dartium, not dart2js.
> 
> One thing I've learned since we launched the new pub build is that users use
it
> quite frequently, even while debugging or otherwise playing around.
> 
> How about I leave this in but add a warning when it gets hit?

A warning is definitely better than nothing. It's you really think this is a net
benefit, leave it in, but I'd much rather see this gone. I don't think we'd
include it if we were writing the dart2js transformer today.

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

https://codereview.chromium.org/207423003/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:246: //
TODO(rnystrom): Fix this if $17751 is fixed.
"$" -> "#"

[Bob Nystrom, 2014-03-25 00:47:28]

Committed patchset #3 manually as r34343 (presubmit successful).

[Bob Nystrom, 2014-03-25 00:49:27]

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

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:232: //
pseudo-package so that it puts correct relative URLs in the source maps.
On 2014/03/24 23:17:14, nweiz wrote:
> On 2014/03/24 22:52:19, Bob Nystrom wrote:
> > On 2014/03/24 21:53:47, nweiz wrote:
> > > On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > > > On 2014/03/21 19:36:06, nweiz wrote:
> > > > > "in the output directory (which is inside the $sdk pseudo-package) so
> > that"
> > > > > 
> > > > > "output directory" is also kind of confusing, because no such thing
> exists
> > > > when
> > > > > using "pub serve".
> > > > 
> > > > Rewrote. Going forward, I think "build directory" should be the
canonical
> > term
> > > > for "directory in entrypoint package added to set of sources for pub
serve
> > and
> > > > build".
> > > 
> > > "Build directory" seems pretty confusing since there's a directory
involved
> in
> > > this process literally named "build".
> > 
> > Yeah, it was the best I could come up with, and mirrors "build environment".
I
> > suppose "transformer environment" would be more precise, but that doesn't
> > exactly roll off the tongue. Reworded the docs a bit here to clarify.
> 
> What about "source directory"? It's up to you, but I really found "build
> directory" confusing when reading the associated documentation.
> 
> > > Whatever terminology you choose, you should update the documentation on
> > > [BuildEnvironment._directories] to reflect it.
> > 
> > It says:
> > 
> > /// The public directories in the root package that are available for
> > /// building, keyed by their root directory.
> > 
> > which sounds about right to me. 
> 
> My point is less that the existing documentation is unclear, more that if
you're
> defining a specific term that would be a nice place to introduce it.

Yeah, I agree. I want to get this patch in soon, so just added a TODO.

https://codereview.chromium.org/207423003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:356: // absolute
file paths in imports that don't touch a package.
On 2014/03/24 23:17:14, nweiz wrote:
> On 2014/03/24 22:52:19, Bob Nystrom wrote:
> > On 2014/03/24 21:53:47, nweiz wrote:
> > > On 2014/03/24 20:45:58, Bob Nystrom wrote:
> > > > On 2014/03/21 19:36:06, nweiz wrote:
> > > > > I think we should stop allowing imports from the filesystem. No other
> > > > > transformer will support it.
> > > > 
> > > > I'm certainly not crazy about this, but if a user has a program that
> > contains
> > > > something like:
> > > > 
> > > > import '/Users/me/some/file.dart';
> > > > 
> > > > The program isn't incorrect. It will run in the VM and will run if you
> > invoke
> > > > dart2js explicitly. In fact, I occasionally do this in hack temp code.
> > > > 
> > > > If it doesn't work in pub, they will file bugs.
> > > 
> > > It will break in confusing ways in Dartium or in the presence of other
> > > transformers. We should fail consistently and as quickly as possible if
they
> > do
> > > something as unportable as this.
> > 
> > I'm sympathetic to this, but my experience with pub build has been that I
keep
> > underestimating the things people want to do with it. I'd like to help them
> > avoid shooting themselves in the foot here. But at the same time, when the
> user
> > *does* know what they're doing, it's a drag if pub pretends to know better
> than
> > they do and prohibits it.
> 
> The thing is, it's going to be prohibited one way or another almost all the
> time. If we add the analyzer as a linting transformer, it will break. If the
> user starts using Polymer, it will break. If the user starts using Dartium, it
> will break. If the user shares the package, it will break.
> 
> I'm also worried about users starting to use this and creating a culture where
> all transformer authors feel obligated to add special-casing for filesystem
> paths in order to be compatible with existing Dart code.
> 
> > > Besides, if users are writing quick hacky code, they won't be running it
> > through
> > > dart2js, they'll be running it in Dartium, not dart2js.
> > 
> > One thing I've learned since we launched the new pub build is that users use
> it
> > quite frequently, even while debugging or otherwise playing around.
> > 
> > How about I leave this in but add a warning when it gets hit?
> 
> A warning is definitely better than nothing. It's you really think this is a
net
> benefit, leave it in, but I'd much rather see this gone. I don't think we'd
> include it if we were writing the dart2js transformer today.

Deleted it.

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

https://codereview.chromium.org/207423003/diff/40001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/barback/dart2js_transformer.dart:246: //
TODO(rnystrom): Fix this if $17751 is fixed.
On 2014/03/24 23:17:14, nweiz wrote:
> "$" -> "#"

Done.