Removing polymer-specific code from barback runner

Removing polymer-specific code from barback runner

Hoping to make this into a generally-usable component.
BUG=
R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2014-02-11 23:15:20]

thanks Pete, this looks great. Just minor comments below

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:198: packagePhases: {'polymer' : phases});
actually, for polymer we only need to apply the observe transformer on a single
file. Could we specify that instead?

In other words, we want something that matches this part of polymer's pubspec:

transformers:
- observe:
    files: lib/src/instance.dart

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/deploy.d...
pkg/polymer/lib/deploy.dart:48: options = new BarbackOptions(phases, outDir,
it's fine in it's current form, but I wonder if we want to make phases here be
also a packagePhase with the key being the current package.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/runner.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:23: /** Phases of transformers to run for
the current package.
nit: fix the multiline style of this comment (extra \n)

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:59: /** Phases of transformers to apply to
packages other than the current
same here (style should be:
/**
 * Phases ...
)

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:66: this.followLinks: false,
packagePhases})
lets add a type for packagePhases here

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:72: ? packageHome :
Directory.current.path),
you seem to already default to Directory.current in the logic below. Consider
just making this null by default?

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:127:
map[readCurrentPackageFromPubspec(packageHome)] = packageHome;
seems that we are computing the current-package twice, could we pass it in?

seems that the old code had the argument, but was buggy (seems like I was
incorrectly passing the argument instead of the field value, so we might need to
move some of the initialization to the body of the constructor to pass the right
thing)

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:187: barback.updateSources(assets);
would you mind adding a small comment here saying that we add the sources after
the transformers so that barback has everything it needs before it starts
processing these assets?

(I got confused briefly because the updateTransformers was before 'addAssets',
but what matters is that it's before updateSources).

[blois, 2014-02-12 02:39:27]

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:198: packagePhases: {'polymer' : phases});
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> actually, for polymer we only need to apply the observe transformer on a
single
> file. Could we specify that instead?
> 
> In other words, we want something that matches this part of polymer's pubspec:
> 
> transformers:
> - observe:
>     files: lib/src/instance.dart

Updated builder & deploy to cover this.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/deploy.d...
pkg/polymer/lib/deploy.dart:48: options = new BarbackOptions(phases, outDir,
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> it's fine in it's current form, but I wonder if we want to make phases here be
> also a packagePhase with the key being the current package.

Yeah- I was considering removing the phases completely, but it seems like it's
the most common- to run the transform on the current package. And since the
current package can be auto-detected it seems lame to have to specify it.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/runner.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:23: /** Phases of transformers to run for
the current package.
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> nit: fix the multiline style of this comment (extra \n)

Done.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:59: /** Phases of transformers to apply to
packages other than the current
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> same here (style should be:
> /**
>  * Phases ...
> )

Done.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:66: this.followLinks: false,
packagePhases})
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> lets add a type for packagePhases here

Done.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:72: ? packageHome :
Directory.current.path),
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> you seem to already default to Directory.current in the logic below. Consider
> just making this null by default?

Done.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:127:
map[readCurrentPackageFromPubspec(packageHome)] = packageHome;
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> seems that we are computing the current-package twice, could we pass it in?
> 
> seems that the old code had the argument, but was buggy (seems like I was
> incorrectly passing the argument instead of the field value, so we might need
to
> move some of the initialization to the body of the constructor to pass the
right
> thing)

Updated to only read if currentPackage is null.

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/runner.dart:187: barback.updateSources(assets);
On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> would you mind adding a small comment here saying that we add the sources
after
> the transformers so that barback has everything it needs before it starts
> processing these assets?
> 
> (I got confused briefly because the updateTransformers was before 'addAssets',
> but what matters is that it's before updateSources).

Done.

[Siggi Cherem (dart-lang), 2014-02-12 03:05:27]

thanks Pete!

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/builder....
File pkg/polymer/lib/builder.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/builder....
pkg/polymer/lib/builder.dart:198: packagePhases: {'polymer' : phases});
On 2014/02/12 02:39:27, blois wrote:
> On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> > actually, for polymer we only need to apply the observe transformer on a
> single
> > file. Could we specify that instead?
> > 
> > In other words, we want something that matches this part of polymer's
pubspec:
> > 
> > transformers:
> > - observe:
> >     files: lib/src/instance.dart
> 
> Updated builder & deploy to cover this.

awesome, thanks!

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/157583007/diff/50001/pkg/polymer/lib/deploy.d...
pkg/polymer/lib/deploy.dart:48: options = new BarbackOptions(phases, outDir,
On 2014/02/12 02:39:27, blois wrote:
> On 2014/02/11 23:15:20, Siggi Cherem (dart-lang) wrote:
> > it's fine in it's current form, but I wonder if we want to make phases here
be
> > also a packagePhase with the key being the current package.
> 
> Yeah- I was considering removing the phases completely, but it seems like it's
> the most common- to run the transform on the current package. And since the
> current package can be auto-detected it seems lame to have to specify it.

sounds good

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/deploy....
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/deploy....
pkg/polymer/lib/deploy.dart:49: packagePhases: {'polymer': phases});
change this one too (phases => polymerPhases)

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:122: List<List<Transformer>> get
polymerPhases =>
should we rename to "phasesForPolymer"? "polymerPhases" sounds like these are
the phases exported by polymer to use on polymer projects.

[blois, 2014-02-12 17:45:25]

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/deploy....
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/deploy....
pkg/polymer/lib/deploy.dart:49: packagePhases: {'polymer': phases});
On 2014/02/12 03:05:28, Siggi Cherem (dart-lang) wrote:
> change this one too (phases => polymerPhases)

D'oh!

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/157583007/diff/130001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:122: List<List<Transformer>> get
polymerPhases =>
On 2014/02/12 03:05:28, Siggi Cherem (dart-lang) wrote:
> should we rename to "phasesForPolymer"? "polymerPhases" sounds like these are
> the phases exported by polymer to use on polymer projects.

Done.

[Siggi Cherem (dart-lang), 2014-02-12 17:46:28]

lgtm! thanks!

[blois, 2014-02-12 17:54:59]

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

[Jennifer Messerly, 2014-02-22 23:48:23]

fyi -- this change seems to have broken how our tests run.

What I'm seeing in tests: the Polymer HTML transformer runs on the polymer
package, which I'm guessing is not intended. It also means we can't add any HTML
files to polymer/lib :|

I'll keep looking but if y'all have any ideas on this, let me know...

[blois, 2014-02-24 16:02:27]

https://codereview.chromium.org/157583007/diff/210001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/runner.dart (right):

https://codereview.chromium.org/157583007/diff/210001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/runner.dart:189:
barback.updateTransformers(options.currentPackage, options.phases);
If Polymer is the currentPackage when building in the Dart repo then yes, it
will be running all polymer transformers on it.

Also- interesting that polymer is not one of the _polymerPackageDependencies.

[Jennifer Messerly, 2014-02-24 20:02:40]

thanks, very helpful

https://codereview.chromium.org/157583007/diff/210001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/runner.dart (right):

https://codereview.chromium.org/157583007/diff/210001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/runner.dart:189:
barback.updateTransformers(options.currentPackage, options.phases);
On 2014/02/24 16:02:27, blois wrote:
> If Polymer is the currentPackage when building in the Dart repo then yes, it
> will be running all polymer transformers on it.
> 
> Also- interesting that polymer is not one of the _polymerPackageDependencies.

aha! good catch :)