added plugin transformer which initializers can extend to do additional post-processing during tran…

added plugin transformer which initializers can extend to do additional post-processing during transformation

R=jmesserly@google.com

Committed: https://github.com/dart-lang/static_init/commit/65e42d70b94f669d25b29d900edaece1120bc6fc

[jakemac, 2015-01-30 19:48:15]

jakemac@google.com changed reviewers:
+ jmesserly@google.com

[jakemac, 2015-01-30 19:48:15]

The formatter did some weird stuff with one part of this, but I decided to leave
it for now. I will submit an issue on the formatter about it.

[Jennifer Messerly, 2015-01-30 20:34:26]

One high level comment, otherwise this is looking great. I like how we've
encapsulated the initialization rewrite, so it should be a lot less fragile
across versions :)

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.dart
File lib/plugin_transformer.dart (right):

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:12: /// Abstract transformer which will call
[initEntry] for every [InitEntry]
could we do the initEntry here instead? it would save writing and re-parsing:
https://github.com/dart-lang/initialize/blob/master/lib/transformer.dart#L355

it seems like we'd need some way for webcomponents to "register" that it wants
to do an init transform.

is the issue that it's hard to do plugins/dependency injection in Dart?

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:28: if (_bootstrapFile == id.path) return
_bootstrapFile;
silly, but I'd usually format this as: 

=> _bootstrapFile == id.path ? _bootstrapFile : null;

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:47: suppressErrors: true) as
dynamic).declarations.firstWhere(
is there a reason for `as dynamic`?

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:48: (d) => d.name.toString() ==
in general not all CompilationUnitMembers have names (e.g.
TopLevelVariableDeclaration because `var x,y,z;) ... perhaps change this to: 

(d) => d is FunctionDeclaration && d.name.name == 'main'


this isn't strictly needed because you know you're parsing a file with a
particular format :), but seems like it wouldn't hurt

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:70: return Future.wait([done.future,
listener.asFuture()]);
if you're returning listener.asFuture(), do you need `done` at all?

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
File test/plugin_transformer_test.dart (right):

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
test/plugin_transformer_test.dart:6: import 'common.dart';
trivial: put this after package: imports?

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
test/plugin_transformer_test.dart:39: new InitEntry(i0.initMethod, const
LibraryIdentifier(#a, null, 'a.dart')),
long line

[jakemac, 2015-01-30 22:16:18]

updated

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.dart
File lib/plugin_transformer.dart (right):

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:12: /// Abstract transformer which will call
[initEntry] for every [InitEntry]
On 2015/01/30 20:34:26, John Messerly wrote:
> could we do the initEntry here instead? it would save writing and re-parsing:
> https://github.com/dart-lang/initialize/blob/master/lib/transformer.dart#L355
> 
> it seems like we'd need some way for webcomponents to "register" that it wants
> to do an init transform.
> 
> is the issue that it's hard to do plugins/dependency injection in Dart?

Yes, especially with transforms since they are just declared in the
pubspec.yaml. We could do this if we required everybody to make their own
transformer which extended the initialize transformer and mixed in some sort of
plugins, but I don't think normal people are going to want to do that.

Alternatively the web_components package could release an html_imports aware
transformer, but then if another package wants to do the same thing I don't see
how those would play nicely.

While not necessarily the most efficient solution, I think this is the easiest
for people to use, they can just list any of these plugin transformers after
their initialize transformer, and they should all play nicely together.

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:28: if (_bootstrapFile == id.path) return
_bootstrapFile;
On 2015/01/30 20:34:26, John Messerly wrote:
> silly, but I'd usually format this as: 
> 
> => _bootstrapFile == id.path ? _bootstrapFile : null;

Done.

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:47: suppressErrors: true) as
dynamic).declarations.firstWhere(
On 2015/01/30 20:34:26, John Messerly wrote:
> is there a reason for `as dynamic`?

This is just to get rid of a bunch of casts and type checks that would be
required otherwise. We know the structure of the file already, and if that
changes this will break appropriately.

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:48: (d) => d.name.toString() ==
On 2015/01/30 20:34:26, John Messerly wrote:
> in general not all CompilationUnitMembers have names (e.g.
> TopLevelVariableDeclaration because `var x,y,z;) ... perhaps change this to: 
> 
> (d) => d is FunctionDeclaration && d.name.name == 'main'
> 
> 
> this isn't strictly needed because you know you're parsing a file with a
> particular format :), but seems like it wouldn't hurt

Related to the previous comment, I did start going down this path originally but
it ended up meaning like 3-4 type checks and casts throughout this expression.
Overall I think its more readable without all of that.

https://codereview.chromium.org/888943002/diff/20001/lib/plugin_transformer.d...
lib/plugin_transformer.dart:70: return Future.wait([done.future,
listener.asFuture()]);
On 2015/01/30 20:34:26, John Messerly wrote:
> if you're returning listener.asFuture(), do you need `done` at all?

Looks like you are right, removed it.

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
File test/plugin_transformer_test.dart (right):

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
test/plugin_transformer_test.dart:6: import 'common.dart';
On 2015/01/30 20:34:26, John Messerly wrote:
> trivial: put this after package: imports?

Done.

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
test/plugin_transformer_test.dart:39: new InitEntry(i0.initMethod, const
LibraryIdentifier(#a, null, 'a.dart')),
On 2015/01/30 20:34:26, John Messerly wrote:
> long line

This is actually intentional in this case, for simplicity these generated files
do violate the 80 character rule. For instance the removeInitializer function
just removes the line that the initializer starts on.

We could potentially hook in the formatter or something but I don't think it
would really provide much benefit for generated files.

[Jennifer Messerly, 2015-01-30 23:11:03]

style nit, otherwise LGTM

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
File test/plugin_transformer_test.dart (right):

https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
test/plugin_transformer_test.dart:39: new InitEntry(i0.initMethod, const
LibraryIdentifier(#a, null, 'a.dart')),
On 2015/01/30 22:16:18, jakemac wrote:
> On 2015/01/30 20:34:26, John Messerly wrote:
> > long line
> 
> This is actually intentional in this case, for simplicity these generated
files
> do violate the 80 character rule. For instance the removeInitializer function
> just removes the line that the initializer starts on.
> 
> We could potentially hook in the formatter or something but I don't think it
> would really provide much benefit for generated files.

right, but this is a string literal in the a human authored test right? and
you're already replacing all leading whitespace ... it seems like the indent
level should just be reduced, then it would be alright as far as style guide is
concerned :)

[jakemac, 2015-02-02 16:29:42]

On 2015/01/30 23:11:03, John Messerly wrote:
> style nit, otherwise LGTM
> 
>
https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
> File test/plugin_transformer_test.dart (right):
> 
>
https://codereview.chromium.org/888943002/diff/20001/test/plugin_transformer_...
> test/plugin_transformer_test.dart:39: new InitEntry(i0.initMethod, const
> LibraryIdentifier(#a, null, 'a.dart')),
> On 2015/01/30 22:16:18, jakemac wrote:
> > On 2015/01/30 20:34:26, John Messerly wrote:
> > > long line
> > 
> > This is actually intentional in this case, for simplicity these generated
> files
> > do violate the 80 character rule. For instance the removeInitializer
function
> > just removes the line that the initializer starts on.
> > 
> > We could potentially hook in the formatter or something but I don't think it
> > would really provide much benefit for generated files.
> 
> right, but this is a string literal in the a human authored test right? and
> you're already replacing all leading whitespace ... it seems like the indent
> level should just be reduced, then it would be alright as far as style guide
is
> concerned :)

done

[jakemac, 2015-02-02 16:37:47]

Committed patchset #5 (id:80001) manually as
65e42d70b94f669d25b29d900edaece1120bc6fc (presubmit successful).