From 23cd385f206aa9e492ea15966b134fea2ef7d9c4 Mon Sep 17 00:00:00 2001 From: Tim Blasi Date: Fri, 24 Jul 2015 11:42:34 -0700 Subject: [PATCH] fix(dart/transform): Handle mixed lifecycle specs Update the transformer to handle classes which both have a `lifecycle` value and `implement` lifecycle interfaces. Closes #3276 --- .../directive_processor/visitors.dart | 156 ++++++++++++------ .../directive_processor/all_tests.dart | 11 +- .../expected/soup.ng_deps.dart | 12 +- .../soup.dart | 6 + 4 files changed, 131 insertions(+), 54 deletions(-) diff --git a/modules/angular2/src/transform/directive_processor/visitors.dart b/modules/angular2/src/transform/directive_processor/visitors.dart index c8cc41afbc..dde4c46aa0 100644 --- a/modules/angular2/src/transform/directive_processor/visitors.dart +++ b/modules/angular2/src/transform/directive_processor/visitors.dart @@ -3,6 +3,7 @@ library angular2.transform.directive_processor.visitors; import 'dart:async'; import 'package:analyzer/analyzer.dart'; import 'package:analyzer/src/generated/java_core.dart'; +import 'package:angular2/annotations.dart' show LifecycleEvent; import 'package:angular2/src/render/xhr.dart' show XHR; import 'package:angular2/src/transform/common/annotation_matcher.dart'; import 'package:angular2/src/transform/common/async_string_writer.dart'; @@ -216,9 +217,11 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { final AssetId _assetId; final bool _inlineViews; final ConstantEvaluator _evaluator = new ConstantEvaluator(); + final Set _ifaceLifecycleEntries = new Set(); + bool _isLifecycleWritten = false; bool _isProcessingView = false; bool _isProcessingDirective = false; - String _lifecycleValue = null; + String _ifaceLifecyclePrefix = ''; AnnotationsTransformVisitor(AsyncStringWriter writer, this._xhr, this._annotationMatcher, this._interfaceMatcher, this._assetId, @@ -231,12 +234,10 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { /// populates `_lifecycleValue` with the appropriate values if so. If none are /// present, `_lifecycleValue` is not modified. void _populateLifecycleValue(ClassDeclaration node) { - var lifecycleEntries = []; - var prefix = ''; var populateImport = (Identifier name) { - if (prefix.isNotEmpty) return; + if (_ifaceLifecyclePrefix.isNotEmpty) return; var import = _interfaceMatcher.getMatchingImport(name, _assetId); - prefix = + _ifaceLifecyclePrefix = import != null && import.prefix != null ? '${import.prefix}.' : ''; }; @@ -254,30 +255,32 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { namesToTest.forEach((name) { if (_interfaceMatcher.isOnChange(name, _assetId)) { - lifecycleEntries.add('onChange'); + _ifaceLifecycleEntries.add('${LifecycleEvent.onChange}'); populateImport(name); } if (_interfaceMatcher.isOnDestroy(name, _assetId)) { - lifecycleEntries.add('onDestroy'); + _ifaceLifecycleEntries.add('${LifecycleEvent.onDestroy}'); populateImport(name); } if (_interfaceMatcher.isOnCheck(name, _assetId)) { - lifecycleEntries.add('onCheck'); + _ifaceLifecycleEntries.add('${LifecycleEvent.onCheck}'); populateImport(name); } if (_interfaceMatcher.isOnInit(name, _assetId)) { - lifecycleEntries.add('onInit'); + _ifaceLifecycleEntries.add('${LifecycleEvent.onInit}'); populateImport(name); } if (_interfaceMatcher.isOnAllChangesDone(name, _assetId)) { - lifecycleEntries.add('onAllChangesDone'); + _ifaceLifecycleEntries.add('${LifecycleEvent.onAllChangesDone}'); populateImport(name); } }); - if (lifecycleEntries.isNotEmpty) { - _lifecycleValue = 'const [${prefix}LifecycleEvent.' - '${lifecycleEntries.join(", ${prefix}LifecycleEvent.")}]'; - } + } + + void _resetState() { + _isLifecycleWritten = _isProcessingView = _isProcessingDirective = false; + _ifaceLifecycleEntries.clear(); + _ifaceLifecyclePrefix = ''; } @override @@ -294,7 +297,7 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { } writer.print(']'); - _lifecycleValue = null; + _resetState(); return null; } @@ -322,61 +325,112 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { } args[i].accept(this); } - if (_lifecycleValue != null && _isProcessingDirective) { - writer.print(''', lifecycle: $_lifecycleValue '''); + if (!_isLifecycleWritten && _isProcessingDirective) { + var lifecycleValue = _getLifecycleValue(); + if (lifecycleValue.isNotEmpty) { + writer.print(', lifecycle: $lifecycleValue'); + _isLifecycleWritten = true; + } } writer.print(')'); } return null; } + String _getLifecycleValue() { + if (_ifaceLifecycleEntries.isNotEmpty) { + var entries = _ifaceLifecycleEntries.toList(); + entries.sort(); + return 'const [${_ifaceLifecyclePrefix}' + '${entries.join(", ${_ifaceLifecyclePrefix}")}]'; + } + return ''; + } + /// These correspond to the annotation parameters. @override Object visitNamedExpression(NamedExpression node) { + if (!_isProcessingView && !_isProcessingDirective) { + return super.visitNamedExpression(node); + } // TODO(kegluneq): Remove this limitation. - if (!_isProcessingView || - node.name is! Label || - node.name.label is! SimpleIdentifier) { + if (node.name is! Label || node.name.label is! SimpleIdentifier) { return super.visitNamedExpression(node); } var keyString = '${node.name.label}'; - if (_inlineViews) { - if (keyString == 'templateUrl') { - // Inline the templateUrl - var url = node.expression.accept(_evaluator); - if (url is String) { - writer.print("template: r'''"); - writer.asyncPrint(_readOrEmptyString(url)); - writer.print("'''"); - - // We keep the templateUrl in case the body of the template includes - // relative urls that might be inlined later on (e.g. @import - // directives or url() css values in style tags). - writer.print(", templateUrl: r'$url'"); - return null; - } else { - logger.warning('template url is not a String $url'); - } - } else if (keyString == 'styleUrls') { - // Inline the styleUrls - var urls = node.expression.accept(_evaluator); - writer.print('styles: const ['); - for (var url in urls) { - if (url is String) { - writer.print("r'''"); - writer.asyncPrint(_readOrEmptyString(url)); - writer.print("''', "); - } else { - logger.warning('style url is not a String ${url}'); - } - } - writer.print(']'); + if (_isProcessingView && _inlineViews) { + var isSuccess = this._inlineView(keyString, node.expression); + if (isSuccess) return null; + } + if (_isProcessingDirective && keyString == 'lifecycle') { + var isSuccess = _populateLifecycleFromNamedExpression(node.expression); + if (isSuccess) { + _isLifecycleWritten = true; + writer.print('lifecycle: ${_getLifecycleValue()}'); return null; + } else { + logger.warning('Failed to parse `lifecycle` value. ' + 'The following `LifecycleEvent`s may not be called: ' + '(${_ifaceLifecycleEntries.join(', ')})'); + _isLifecycleWritten = true; + // Do not return -- we will use the default processing here, maintaining + // the original value for `lifecycle`. } } return super.visitNamedExpression(node); } + /// Populates the lifecycle values from explicitly declared values. + /// Returns whether `node` was successfully processed. + bool _populateLifecycleFromNamedExpression(AstNode node) { + var nodeVal = node.toSource(); + for (var evt in LifecycleEvent.values) { + var evtStr = '$evt'; + if (nodeVal.contains(evtStr)) { + _ifaceLifecycleEntries.add(evtStr); + } + } + return true; + } + + /// Inlines the template and/or style refered to by `keyString`. + /// Returns whether the `keyString` value was successfully processed. + bool _inlineView(String keyString, AstNode node) { + if (keyString == 'templateUrl') { + // Inline the templateUrl + var url = node.accept(_evaluator); + if (url is String) { + writer.print("template: r'''"); + writer.asyncPrint(_readOrEmptyString(url)); + writer.print("'''"); + + // We keep the templateUrl in case the body of the template includes + // relative urls that might be inlined later on (e.g. @import + // directives or url() css values in style tags). + writer.print(", templateUrl: r'$url'"); + return true; + } else { + logger.warning('template url is not a String $url'); + } + } else if (keyString == 'styleUrls') { + // Inline the styleUrls + var urls = node.accept(_evaluator); + writer.print('styles: const ['); + for (var url in urls) { + if (url is String) { + writer.print("r'''"); + writer.asyncPrint(_readOrEmptyString(url)); + writer.print("''', "); + } else { + logger.warning('style url is not a String ${url}'); + } + } + writer.print(']'); + return true; + } + return false; + } + /// Attempts to read the content from {@link url}, if it returns null then /// just return the empty string. Future _readOrEmptyString(String url) async { diff --git a/modules/angular2/test/transform/directive_processor/all_tests.dart b/modules/angular2/test/transform/directive_processor/all_tests.dart index 0d59d3b05d..eb7207b251 100644 --- a/modules/angular2/test/transform/directive_processor/all_tests.dart +++ b/modules/angular2/test/transform/directive_processor/all_tests.dart @@ -165,7 +165,9 @@ void _testProcessor(String name, String inputPath, } }); - if (expectedLogs != null) { + if (expectedLogs == null) { + expect(logger.hasErrors).toBeFalse(); + } else { expect(logger.logs, expectedLogs); } }); @@ -180,6 +182,8 @@ class RecordingLogger implements BuildLogger { @override final bool convertErrorsToWarnings = false; + bool hasErrors = false; + List logs = []; void _record(prefix, msg) => logs.add('$prefix: $msg'); @@ -190,7 +194,10 @@ class RecordingLogger implements BuildLogger { void warning(msg, {AssetId asset, SourceSpan span}) => _record('WARN', msg); - void error(msg, {AssetId asset, SourceSpan span}) => _record('ERROR', msg); + void error(msg, {AssetId asset, SourceSpan span}) { + hasErrors = true; + _record('ERROR', msg); + } Future writeOutput() => throw new UnimplementedError(); Future addLogFilesFromAsset(AssetId id, [int nextNumber = 1]) => diff --git a/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/expected/soup.ng_deps.dart b/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/expected/soup.ng_deps.dart index 6e0196aab4..9d53870497 100644 --- a/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/expected/soup.ng_deps.dart +++ b/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/expected/soup.ng_deps.dart @@ -22,5 +22,15 @@ void initReflector() { OnChange, OnDestroy, OnInit - ])); + ])) + ..registerType(MixedSoupComponent, new _ngRef.ReflectionInfo(const [ + const Component( + selector: '[soup]', + lifecycle: const [LifecycleEvent.onChange, LifecycleEvent.onCheck]) + ], const [], () => new MixedSoupComponent(), const [OnChange])) + ..registerType(MatchedSoupComponent, new _ngRef.ReflectionInfo(const [ + const Component( + selector: '[soup]', + lifecycle: const [LifecycleEvent.onChange]) + ], const [], () => new MatchedSoupComponent(), const [OnChange])); } diff --git a/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/soup.dart b/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/soup.dart index 8ef9fb91fb..a4c3ed88a6 100644 --- a/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/soup.dart +++ b/modules/angular2/test/transform/directive_processor/multiple_interface_lifecycle_files/soup.dart @@ -4,3 +4,9 @@ import 'package:angular2/annotations.dart'; @Component(selector: '[soup]') class MultiSoupComponent implements OnChange, OnDestroy, OnInit {} + +@Component(selector: '[soup]', lifecycle: const [LifecycleEvent.onCheck]) +class MixedSoupComponent implements OnChange {} + +@Component(selector: '[soup]', lifecycle: const [LifecycleEvent.onChange]) +class MatchedSoupComponent implements OnChange {}