From bdd031aecc639f77b82c7065215adffad746d932 Mon Sep 17 00:00:00 2001 From: Tim Blasi Date: Tue, 20 Oct 2015 08:46:41 -0700 Subject: [PATCH] feat(dart/transform): Match runtime semantics for template values Match [ViewResolver][]'s semantics for reading template and style values from `@Component` and `@View` annotations. We now warn if template and/or style values appear on an `@Component` annotation and a `@View` annotation is present. [ViewResolver]: https://github.com/angular/angular/blob/7c6130c2c5d8ac2d5ba8df5f70b7f1264c283a2c/modules/angular2/src/core/linker/view_resolver.ts --- .../common/code/reflection_info_code.dart | 40 +++++++++++------ .../common/directive_metadata_reader.dart | 43 +++++++++++++++---- .../directive_processor/all_tests.dart | 38 ++++++++++++++++ .../bad_directives_files/directives.dart | 10 +++++ .../bad_directives_files/template.dart | 10 +++++ .../bad_directives_files/template_url.dart | 10 +++++ 6 files changed, 129 insertions(+), 22 deletions(-) create mode 100644 modules_dart/transform/test/transform/directive_processor/bad_directives_files/directives.dart create mode 100644 modules_dart/transform/test/transform/directive_processor/bad_directives_files/template.dart create mode 100644 modules_dart/transform/test/transform/directive_processor/bad_directives_files/template_url.dart diff --git a/modules_dart/transform/lib/src/transform/common/code/reflection_info_code.dart b/modules_dart/transform/lib/src/transform/common/code/reflection_info_code.dart index 9fa1e2b761..86568efe1e 100644 --- a/modules_dart/transform/lib/src/transform/common/code/reflection_info_code.dart +++ b/modules_dart/transform/lib/src/transform/common/code/reflection_info_code.dart @@ -25,9 +25,6 @@ class ReflectionInfoVisitor extends RecursiveAstVisitor { final ParameterVisitor _parameterVisitor = new ParameterVisitor(); final _PropertyMetadataVisitor _propMetadataVisitor; - /// Whether an Angular 2 `Reflection` has been found. - bool _foundNgReflection = false; - ReflectionInfoVisitor._(this.assetId, this._annotationMatcher, this._annotationVisitor, this._propMetadataVisitor); @@ -38,8 +35,6 @@ class ReflectionInfoVisitor extends RecursiveAstVisitor { annotationVisitor, new _PropertyMetadataVisitor(annotationVisitor)); } - bool get shouldCreateNgDeps => _foundNgReflection; - ConstructorDeclaration _getCtor(ClassDeclaration node) { int numCtorsFound = 0; var ctor = null; @@ -82,13 +77,26 @@ class ReflectionInfoVisitor extends RecursiveAstVisitor { } if (node.metadata != null) { + var componentDirectives, viewDirectives; node.metadata.forEach((node) { - final extractedDirectives = _extractDirectives(node); - if (extractedDirectives != null) { - model.directives.addAll(extractedDirectives); + if (_annotationMatcher.isComponent(node, assetId)) { + componentDirectives = _extractDirectives(node); + } else if (_annotationMatcher.isView(node, assetId)) { + viewDirectives = _extractDirectives(node); } model.annotations.add(_annotationVisitor.visitAnnotation(node)); }); + if (componentDirectives != null && componentDirectives.isNotEmpty) { + if (viewDirectives != null) { + logger.warning( + 'Cannot specify view parameters on @Component when a @View ' + 'is present. Component name: ${model.name}', + asset: assetId); + } + model.directives.addAll(componentDirectives); + } else if (viewDirectives != null) { + model.directives.addAll(viewDirectives); + } } if (ctor != null && ctor.parameters != null && @@ -141,20 +149,26 @@ class ReflectionInfoVisitor extends RecursiveAstVisitor { } } + /// Returns [PrefixedDirective] values parsed from the value of the + /// `directives` parameter of the provided `node`. + /// This will always return a non-null value, so if there are no `directives` + /// specified on `node`, it will return an empty iterable. Iterable _extractDirectives(Annotation node) { - var shouldProcess = _annotationMatcher.isComponent(node, assetId); - shouldProcess = shouldProcess || _annotationMatcher.isView(node, assetId); + assert(_annotationMatcher.isComponent(node, assetId) || + _annotationMatcher.isView(node, assetId)); - if (node.arguments == null && node.arguments.arguments == null) return null; + if (node.arguments == null && node.arguments.arguments == null) { + return const []; + } final directivesNode = node.arguments.arguments.firstWhere((arg) { return arg is NamedExpression && '${arg.name.label}' == 'directives'; }, orElse: () => null); - if (directivesNode == null) return null; + if (directivesNode == null) return const []; if (directivesNode.expression is! ListLiteral) { logger.warning('Angular 2 expects a list literal for `directives` ' 'but found a ${directivesNode.expression.runtimeType}'); - return null; + return const []; } final directives = []; for (var dep in (directivesNode.expression as ListLiteral).elements) { diff --git a/modules_dart/transform/lib/src/transform/common/directive_metadata_reader.dart b/modules_dart/transform/lib/src/transform/common/directive_metadata_reader.dart index 1fd0df7ca6..ef325acfd2 100644 --- a/modules_dart/transform/lib/src/transform/common/directive_metadata_reader.dart +++ b/modules_dart/transform/lib/src/transform/common/directive_metadata_reader.dart @@ -10,6 +10,7 @@ import 'package:angular2/src/core/linker/interfaces.dart' show LifecycleHooks; import 'package:angular2/src/core/metadata/view.dart' show ViewEncapsulation; import 'package:angular2/src/transform/common/annotation_matcher.dart'; import 'package:angular2/src/transform/common/interface_matcher.dart'; +import 'package:angular2/src/transform/common/logging.dart'; import 'package:barback/barback.dart' show AssetId; import 'naive_eval.dart'; @@ -173,6 +174,21 @@ class _DirectiveMetadataVisitor extends Object template: _template); } + /// Ensures that we do not specify View values on an `@Component` annotation + /// when there is a @View annotation present. + void _validateTemplates() { + if (_cmpTemplate != null && _viewTemplate != null) { + var name = '(Unknown)'; + if (_type != null && _type.name != null && _type.name.isNotEmpty) { + name = _type.name; + } + logger.warning( + 'Cannot specify view parameters on @Component when a @View ' + 'is present. Component name: ${name}', + asset: _assetId); + } + } + @override Object visitAnnotation(Annotation node) { var isComponent = _annotationMatcher.isComponent(node, _assetId); @@ -187,14 +203,14 @@ class _DirectiveMetadataVisitor extends Object _isComponent = isComponent; _hasMetadata = true; if (isComponent) { - var t = new _CompileTemplateMetadataVisitor().visitAnnotation(node); - if (t.template != null || t.templateUrl != null) { - _cmpTemplate = t; - } + _cmpTemplate = + new _CompileTemplateMetadataVisitor().visitAnnotation(node); + _validateTemplates(); } super.visitAnnotation(node); } else if (_annotationMatcher.isView(node, _assetId)) { if (_viewTemplate != null) { + // TODO(kegluneq): Support multiple views on a single class. throw new FormatException( 'Only one View is allowed per class. ' 'Found unexpected "$node".', @@ -202,6 +218,7 @@ class _DirectiveMetadataVisitor extends Object } _viewTemplate = new _CompileTemplateMetadataVisitor().visitAnnotation(node); + _validateTemplates(); } // Annotation we do not recognize - no need to visit. @@ -353,16 +370,24 @@ class _LifecycleHookVisitor extends SimpleAstVisitor> { /// [CompileTemplateMetadata]. class _CompileTemplateMetadataVisitor extends RecursiveAstVisitor { - ViewEncapsulation _encapsulation = ViewEncapsulation.Emulated; - String _template = null; - String _templateUrl = null; - List _styles = null; - List _styleUrls = null; + ViewEncapsulation _encapsulation; + String _template; + String _templateUrl; + List _styles; + List _styleUrls; @override CompileTemplateMetadata visitAnnotation(Annotation node) { super.visitAnnotation(node); + if (_encapsulation == null && + _template == null && + _templateUrl == null && + _styles == null && + _styleUrls == null) { + return null; + } + return new CompileTemplateMetadata( encapsulation: _encapsulation, template: _template, diff --git a/modules_dart/transform/test/transform/directive_processor/all_tests.dart b/modules_dart/transform/test/transform/directive_processor/all_tests.dart index ac2fbd218d..9597cd318f 100644 --- a/modules_dart/transform/test/transform/directive_processor/all_tests.dart +++ b/modules_dart/transform/test/transform/directive_processor/all_tests.dart @@ -493,6 +493,44 @@ void allTests() { ..name = 'Dep' ..prefix = 'dep2'); }); + + it('should warn if @Component has a `template` and @View is present.', + () async { + final logger = new RecordingLogger(); + final model = await _testCreateModel('bad_directives_files/template.dart', + logger: logger); + var warning = + logger.logs.firstWhere((l) => l.contains('WARN'), orElse: () => null); + expect(warning).toBeNotNull(); + expect(warning.toLowerCase()) + .toContain('cannot specify view parameters on @component'); + }); + + it('should warn if @Component has a `templateUrl` and @View is present.', + () async { + final logger = new RecordingLogger(); + final model = await _testCreateModel( + 'bad_directives_files/template_url.dart', + logger: logger); + var warning = + logger.logs.firstWhere((l) => l.contains('WARN'), orElse: () => null); + expect(warning).toBeNotNull(); + expect(warning.toLowerCase()) + .toContain('cannot specify view parameters on @component'); + }); + + it('should warn if @Component has `directives` and @View is present.', + () async { + final logger = new RecordingLogger(); + final model = await _testCreateModel( + 'bad_directives_files/directives.dart', + logger: logger); + var warning = + logger.logs.firstWhere((l) => l.contains('WARN'), orElse: () => null); + expect(warning).toBeNotNull(); + expect(warning.toLowerCase()) + .toContain('cannot specify view parameters on @component'); + }); }); } diff --git a/modules_dart/transform/test/transform/directive_processor/bad_directives_files/directives.dart b/modules_dart/transform/test/transform/directive_processor/bad_directives_files/directives.dart new file mode 100644 index 0000000000..dd91efd9d0 --- /dev/null +++ b/modules_dart/transform/test/transform/directive_processor/bad_directives_files/directives.dart @@ -0,0 +1,10 @@ +library angular2.test.transform.directive_processor.bad_directives_files.directives; + +import 'package:angular2/angular2.dart' + show Component, Directive, View, NgElement; +import 'dep1.dart'; +import 'dep2.dart' as dep2; + +@Component(selector: 'component-first', directives: [Dep, dep2.Dep]) +@View(template: '
Hi
') +class BadDirectives {} diff --git a/modules_dart/transform/test/transform/directive_processor/bad_directives_files/template.dart b/modules_dart/transform/test/transform/directive_processor/bad_directives_files/template.dart new file mode 100644 index 0000000000..3bbd6f2e97 --- /dev/null +++ b/modules_dart/transform/test/transform/directive_processor/bad_directives_files/template.dart @@ -0,0 +1,10 @@ +library angular2.test.transform.directive_processor.bad_directives_files.template; + +import 'package:angular2/angular2.dart' + show Component, Directive, View, NgElement; +import 'dep1.dart'; +import 'dep2.dart' as dep2; + +@Component(selector: 'component-first', template: 'bad!') +@View(template: 'good!', directives: [Dep, dep2.Dep]) +class BadTemplate {} diff --git a/modules_dart/transform/test/transform/directive_processor/bad_directives_files/template_url.dart b/modules_dart/transform/test/transform/directive_processor/bad_directives_files/template_url.dart new file mode 100644 index 0000000000..c4058bc6f9 --- /dev/null +++ b/modules_dart/transform/test/transform/directive_processor/bad_directives_files/template_url.dart @@ -0,0 +1,10 @@ +library angular2.test.transform.directive_processor.bad_directives_files.template_url; + +import 'package:angular2/angular2.dart' + show Component, Directive, View, NgElement; +import 'dep1.dart'; +import 'dep2.dart' as dep2; + +@Component(selector: 'component-first', templateUrl: 'bad_template.html') +@View(templateUrl: 'good_template.html', directives: [Dep, dep2.Dep]) +class BadTemplateUrl {}