Initial commit of Dart transformer to generate constructor stubs, see https://github.com/angular/an…

Initial commit of Dart transformer to generate constructor stubs, see https://github.com/angular/angular/issues/496.

Add some basic code for generating the "annotations" property.
See https://github.com/angular/angular/issues/498.

BUG=

[tjblasi, 2015-02-17 21:19:26]

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

[tjblasi, 2015-02-17 21:19:27]

I have a big favor to ask - would you mind reviewing this code? Since it's
modified package:initialize I think you have the most context. Sorry it's such a
big diff...

Let me know if you're swamped and I'll find somebody else to send to.

[jakemac, 2015-02-17 23:46:46]

just did a quick pass, I can come back and do a more in depth one tomorrow

https://codereview.chromium.org/927373004/diff/1/modules/angular2/pubspec.yaml
File modules/angular2/pubspec.yaml (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/pubspec.yam...
modules/angular2/pubspec.yaml:12: analyzer: '0.22.4'
These should probably all be ranges up to the next non-breaking version

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/annotation_processor.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/annotation_processor.dart:1: import
'dart:collection' show Queue;
Should probably add a library/license (ditto other files)

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/annotation_processor.dart:8: final initQueue =
new Queue<AnnotationMatch>();
nit: may want to rename this

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/annotation_processor.dart:19: /// 2) ([element],
[_annotationClass]) has been seen previously.
has -> has not?

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/codegen.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:29: if (_logger != null) {
nit, you could rewrite this:

if (_logger == null) throw new Error(errorString);
_logger.error(errorString);

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:130: if (element.node is!
ClassDeclaration) {
Just fyi, any call to `.node` is potentially pretty expensive. You probably want
to store these in a variable that gets cached. I recenty did this do the
InitializerData class in the initialize transformer,
https://github.com/dart-lang/initialize/blob/master/lib/transformer.dart#L461. I
don't have a good idea of exactly how much of an issue this is, so I will leave
it up to your best judgment as to whether its worth updating now, but I would at
least put in a TODO.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:201: /// @param node the node to be
visited
I haven't really seen us use these JavaDoc style comments, does the doc
generator support them?

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:208: /**
Usually we just do the /// style comments, not sure if this is against style
guides or not.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/transformer.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/transformer.dart:27: static const
_entryPointParam = 'entry_point';
not sure that these are necessary, they only appear once below

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/traversal.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/traversal.dart:6: class ImportTraversal {
ImportTraverser maybe?

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/traversal.dart:28: if
(_annotationMatcher.processAnnotations(superClass.element) &&
I think that for angular this might not matter, as long as super classes don't
have to be registered before child classes.

[jakemac, 2015-02-18 16:18:50]

Ok had another fresh look this morning, everything mostly looks good. When you
get in we should talk though about the traversal strategy as it doesn't seem to
be what I originally understood it to be.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/codegen.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:19: DirectiveRegistry
_directiveRegistry;
might be nice to have a comment here

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:41: var prefix = lib != null &&
!lib.isInSdk
I might reorganize this a little bit to return early for the null case:

if (lib == null || lib.isInSdk) return '';
return _libraryPrefixes.putIfAbsent(
    lib, () => '$i{_libraryPrefixes.length}');

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:48: abstract class DirectiveRegistry
{
A comment here would be good

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:53: const _reflectorImport =
You might just want to use ''' here

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/html_transform.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/html_transform.dart:15: // TODO(jakemac): support
package urls with [options.entryPoint] or
The initialize package now handles this, up to you if you want to copy the logic
over. Basically it just accepts an `entry_point` now and based on the file
extension (.dart or .html) it does the right thing.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/traversal.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/traversal.dart:13: void traverse(LibraryElement
library, [Set<LibraryElement> seen]) {
Looks to me like this is still doing the normal traversal that `initialize`
does. Did the design change on that for angular? If so we could get rid of a ton
of this code potentially by converting it to an InitializerPlugin.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/test/transf...
File modules/angular2/test/transform/transform_test.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/test/transf...
modules/angular2/test/transform/transform_test.dart:25: class TestConfig {
Personally I kind of like just inlining these files, and it makes the tests run
faster. I don't feel that strongly though.

[tjblasi, 2015-02-18 21:18:26]

Thanks for the review -- no need to re-review if you don't want to, just wanted
to get some additional eyes on the code.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/annotation_processor.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/annotation_processor.dart:1: import
'dart:collection' show Queue;
On 2015/02/17 23:46:45, jakemac wrote:
> Should probably add a library/license (ditto other files)

None of the other files seem to have licenses right now, so I'll leave it off
for the time being.

Added libraries.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/annotation_processor.dart:8: final initQueue =
new Queue<AnnotationMatch>();
On 2015/02/17 23:46:45, jakemac wrote:
> nit: may want to rename this

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/annotation_processor.dart:19: /// 2) ([element],
[_annotationClass]) has been seen previously.
On 2015/02/17 23:46:45, jakemac wrote:
> has -> has not?

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/codegen.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:19: DirectiveRegistry
_directiveRegistry;
On 2015/02/18 16:18:50, jakemac wrote:
> might be nice to have a comment here

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:29: if (_logger != null) {
On 2015/02/17 23:46:45, jakemac wrote:
> nit, you could rewrite this:
> 
> if (_logger == null) throw new Error(errorString);
> _logger.error(errorString);

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:41: var prefix = lib != null &&
!lib.isInSdk
On 2015/02/18 16:18:50, jakemac wrote:
> I might reorganize this a little bit to return early for the null case:
> 
> if (lib == null || lib.isInSdk) return '';
> return _libraryPrefixes.putIfAbsent(
>     lib, () => '$i{_libraryPrefixes.length}');

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:48: abstract class DirectiveRegistry
{
On 2015/02/18 16:18:50, jakemac wrote:
> A comment here would be good

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:53: const _reflectorImport =
On 2015/02/18 16:18:50, jakemac wrote:
> You might just want to use ''' here

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:130: if (element.node is!
ClassDeclaration) {
On 2015/02/17 23:46:46, jakemac wrote:
> Just fyi, any call to `.node` is potentially pretty expensive. You probably
want
> to store these in a variable that gets cached. I recenty did this do the
> InitializerData class in the initialize transformer,
> https://github.com/dart-lang/initialize/blob/master/lib/transformer.dart#L461.
I
> don't have a good idea of exactly how much of an issue this is, so I will
leave
> it up to your best judgment as to whether its worth updating now, but I would
at
> least put in a TODO.

Added an issue [https://github.com/kegluneq/angular/issues/4] & TODO.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:201: /// @param node the node to be
visited
On 2015/02/17 23:46:45, jakemac wrote:
> I haven't really seen us use these JavaDoc style comments, does the doc
> generator support them?

Ah, I stole this code from analyzer, which was generated from Java code. Fixed.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/codegen.dart:208: /**
On 2015/02/17 23:46:46, jakemac wrote:
> Usually we just do the /// style comments, not sure if this is against style
> guides or not.

Done.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/html_transform.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/html_transform.dart:15: // TODO(jakemac): support
package urls with [options.entryPoint] or
On 2015/02/18 16:18:50, jakemac wrote:
> The initialize package now handles this, up to you if you want to copy the
logic
> over. Basically it just accepts an `entry_point` now and based on the file
> extension (.dart or .html) it does the right thing.

Acknowledged.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/transformer.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/transformer.dart:27: static const
_entryPointParam = 'entry_point';
On 2015/02/17 23:46:46, jakemac wrote:
> not sure that these are necessary, they only appear once below

They're probably going to change in the future -- this helps me keep them
straight.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
File modules/angular2/src/transform/traversal.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/traversal.dart:6: class ImportTraversal {
On 2015/02/17 23:46:46, jakemac wrote:
> ImportTraverser maybe?

Acknowledged.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/traversal.dart:13: void traverse(LibraryElement
library, [Set<LibraryElement> seen]) {
On 2015/02/18 16:18:50, jakemac wrote:
> Looks to me like this is still doing the normal traversal that `initialize`
> does. Did the design change on that for angular? If so we could get rid of a
ton
> of this code potentially by converting it to an InitializerPlugin.

Discussed offline

https://codereview.chromium.org/927373004/diff/1/modules/angular2/src/transfo...
modules/angular2/src/transform/traversal.dart:28: if
(_annotationMatcher.processAnnotations(superClass.element) &&
On 2015/02/17 23:46:46, jakemac wrote:
> I think that for angular this might not matter, as long as super classes don't
> have to be registered before child classes.

Acknowledged.

https://codereview.chromium.org/927373004/diff/1/modules/angular2/test/transf...
File modules/angular2/test/transform/transform_test.dart (right):

https://codereview.chromium.org/927373004/diff/1/modules/angular2/test/transf...
modules/angular2/test/transform/transform_test.dart:25: class TestConfig {
On 2015/02/18 16:18:50, jakemac wrote:
> Personally I kind of like just inlining these files, and it makes the tests
run
> faster. I don't feel that strongly though.

I'm pretty ambivalent, but Misko strongly preferred having them separated out.