diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 5bf87431e6..258b94b473 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -89,17 +89,18 @@ export class DecorationAnalyzer { this.scopeRegistry, this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs, /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true, /* i18nLegacyMessageIdFormat */ '', this.moduleResolver, - this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER), + this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, + /* annotateForClosureCompiler */ false), new DirectiveDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, NOOP_DEFAULT_IMPORT_RECORDER, - this.isCore), + this.isCore, /* annotateForClosureCompiler */ false), new InjectableDecoratorHandler( this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore, /* strictCtorDeps */ false), new NgModuleDecoratorHandler( this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry, this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null, - this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER), + this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false), new PipeDecoratorHandler( this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 560f5dde95..7706e29874 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -28,7 +28,7 @@ import {ResourceLoader} from './api'; import {extractDirectiveMetadata, parseFieldArrayValue} from './directive'; import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, readBaseClass, unwrapExpression} from './util'; +import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; const EMPTY_MAP = new Map(); const EMPTY_ARRAY: any[] = []; @@ -55,6 +55,7 @@ export class ComponentDecoratorHandler implements private i18nLegacyMessageIdFormat: string, private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer, private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder, + private annotateForClosureCompiler: boolean, private resourceDependencies: ResourceDependencyRecorder = new NoopResourceDependencyRecorder()) {} @@ -146,7 +147,8 @@ export class ComponentDecoratorHandler implements // on it. const directiveResult = extractDirectiveMetadata( node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore, - flags, this.elementSchemaRegistry.getDefaultComponentElementName()); + flags, this.annotateForClosureCompiler, + this.elementSchemaRegistry.getDefaultComponentElementName()); if (directiveResult === undefined) { // `extractDirectiveMetadata` returns undefined when the @Directive has `jit: true`. In this // case, compilation of the decorator is skipped. Returning an empty object signifies @@ -169,7 +171,10 @@ export class ComponentDecoratorHandler implements }, undefined) !; const viewProviders: Expression|null = component.has('viewProviders') ? - new WrappedNodeExpr(component.get('viewProviders') !) : + new WrappedNodeExpr( + this.annotateForClosureCompiler ? + wrapFunctionExpressionsInParens(component.get('viewProviders') !) : + component.get('viewProviders') !) : null; // Parse the template. @@ -297,7 +302,8 @@ export class ComponentDecoratorHandler implements i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath }, metadataStmt: generateSetClassMetadataCall( - node, this.reflector, this.defaultImportRecorder, this.isCore), + node, this.reflector, this.defaultImportRecorder, this.isCore, + this.annotateForClosureCompiler), parsedTemplate: template, parseTemplate, templateSourceMapping, }, typeCheck: true, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 6a918a9b21..38688f5ba8 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -19,7 +19,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFl import {compileNgFactoryDefField} from './factory'; import {generateSetClassMetadataCall} from './metadata'; -import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies} from './util'; +import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens} from './util'; const EMPTY_OBJECT: {[key: string]: string} = {}; const FIELD_DECORATORS = [ @@ -40,7 +40,7 @@ export class DirectiveDecoratorHandler implements constructor( private reflector: ReflectionHost, private evaluator: PartialEvaluator, private metaRegistry: MetadataRegistry, private defaultImportRecorder: DefaultImportRecorder, - private isCore: boolean) {} + private isCore: boolean, private annotateForClosureCompiler: boolean) {} readonly precedence = HandlerPrecedence.PRIMARY; @@ -76,7 +76,7 @@ export class DirectiveDecoratorHandler implements AnalysisOutput { const directiveResult = extractDirectiveMetadata( node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore, - flags); + flags, this.annotateForClosureCompiler); const analysis = directiveResult && directiveResult.metadata; if (analysis === undefined) { @@ -102,7 +102,8 @@ export class DirectiveDecoratorHandler implements analysis: { meta: analysis, metadataStmt: generateSetClassMetadataCall( - node, this.reflector, this.defaultImportRecorder, this.isCore), + node, this.reflector, this.defaultImportRecorder, this.isCore, + this.annotateForClosureCompiler), } }; } @@ -136,7 +137,8 @@ export class DirectiveDecoratorHandler implements export function extractDirectiveMetadata( clazz: ClassDeclaration, decorator: Decorator | null, reflector: ReflectionHost, evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean, - flags: HandlerFlags, defaultSelector: string | null = null): { + flags: HandlerFlags, annotateForClosureCompiler: boolean, + defaultSelector: string | null = null): { decorator: Map, metadata: R3DirectiveMetadata, }|undefined { @@ -230,8 +232,12 @@ export function extractDirectiveMetadata( const host = extractHostBindings(decoratedElements, evaluator, coreModule, directive); - const providers: Expression|null = - directive.has('providers') ? new WrappedNodeExpr(directive.get('providers') !) : null; + const providers: Expression|null = directive.has('providers') ? + new WrappedNodeExpr( + annotateForClosureCompiler ? + wrapFunctionExpressionsInParens(directive.get('providers') !) : + directive.get('providers') !) : + null; // Determine if `ngOnChanges` is a lifecycle hook defined on the component. const usesOnChanges = members.some( diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts b/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts index 589561230e..e779f8d817 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts @@ -12,7 +12,7 @@ import * as ts from 'typescript'; import {DefaultImportRecorder} from '../../imports'; import {CtorParameter, Decorator, ReflectionHost} from '../../reflection'; -import {valueReferenceToExpression} from './util'; +import {valueReferenceToExpression, wrapFunctionExpressionsInParens} from './util'; /** @@ -24,7 +24,7 @@ import {valueReferenceToExpression} from './util'; */ export function generateSetClassMetadataCall( clazz: ts.Declaration, reflection: ReflectionHost, defaultImportRecorder: DefaultImportRecorder, - isCore: boolean): Statement|null { + isCore: boolean, annotateForClosureCompiler?: boolean): Statement|null { if (!reflection.isClass(clazz)) { return null; } @@ -37,7 +37,9 @@ export function generateSetClassMetadataCall( return null; } const ngClassDecorators = - classDecorators.filter(dec => isAngularDecorator(dec, isCore)).map(decoratorToMetadata); + classDecorators.filter(dec => isAngularDecorator(dec, isCore)) + .map( + (decorator: Decorator) => decoratorToMetadata(decorator, annotateForClosureCompiler)); if (ngClassDecorators.length === 0) { return null; } @@ -113,8 +115,8 @@ function ctorParameterToMetadata( // If the parameter has decorators, include the ones from Angular. if (param.decorators !== null) { - const ngDecorators = - param.decorators.filter(dec => isAngularDecorator(dec, isCore)).map(decoratorToMetadata); + const ngDecorators = param.decorators.filter(dec => isAngularDecorator(dec, isCore)) + .map((decorator: Decorator) => decoratorToMetadata(decorator)); const value = new WrappedNodeExpr(ts.createArrayLiteral(ngDecorators)); mapEntries.push({key: 'decorators', value, quoted: false}); } @@ -126,8 +128,8 @@ function ctorParameterToMetadata( */ function classMemberToMetadata( name: string, decorators: Decorator[], isCore: boolean): ts.PropertyAssignment { - const ngDecorators = - decorators.filter(dec => isAngularDecorator(dec, isCore)).map(decoratorToMetadata); + const ngDecorators = decorators.filter(dec => isAngularDecorator(dec, isCore)) + .map((decorator: Decorator) => decoratorToMetadata(decorator)); const decoratorMeta = ts.createArrayLiteral(ngDecorators); return ts.createPropertyAssignment(name, decoratorMeta); } @@ -135,7 +137,8 @@ function classMemberToMetadata( /** * Convert a reflected decorator to metadata. */ -function decoratorToMetadata(decorator: Decorator): ts.ObjectLiteralExpression { +function decoratorToMetadata( + decorator: Decorator, wrapFunctionsInParens?: boolean): ts.ObjectLiteralExpression { if (decorator.identifier === null) { throw new Error('Illegal state: synthesized decorator cannot be emitted in class metadata.'); } @@ -145,7 +148,10 @@ function decoratorToMetadata(decorator: Decorator): ts.ObjectLiteralExpression { ]; // Sometimes they have arguments. if (decorator.args !== null && decorator.args.length > 0) { - const args = decorator.args.map(arg => ts.getMutableClone(arg)); + const args = decorator.args.map(arg => { + const expr = ts.getMutableClone(arg); + return wrapFunctionsInParens ? wrapFunctionExpressionsInParens(expr) : expr; + }); properties.push(ts.createPropertyAssignment('args', ts.createArrayLiteral(args))); } return ts.createObjectLiteral(properties, true); diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 8df4671232..3268872c30 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -21,7 +21,7 @@ import {getSourceFile} from '../../util/src/typescript'; import {generateSetClassMetadataCall} from './metadata'; import {ReferencesRegistry} from './references_registry'; -import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression} from './util'; +import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens} from './util'; export interface NgModuleAnalysis { mod: R3NgModuleMetadata; @@ -44,7 +44,8 @@ export class NgModuleDecoratorHandler implements DecoratorHandler = + (context: ts.TransformationContext) => { + const visitor: ts.Visitor = (node: ts.Node): ts.Node => { + const visited = ts.visitEachChild(node, visitor, context); + if (ts.isArrowFunction(visited) || ts.isFunctionExpression(visited)) { + return ts.createParen(visited); + } + return visited; + }; + return (node: ts.Expression) => ts.visitEachChild(node, visitor, context); + }; + +/** + * Wraps all functions in a given expression in parentheses. This is needed to avoid problems + * where Tsickle annotations added between analyse and transform phases in Angular may trigger + * automatic semicolon insertion, e.g. if a function is the expression in a `return` statement. More + * info can be found in Tsickle source code here: + * https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021 + * + * @param expression Expression where functions should be wrapped in parentheses + */ +export function wrapFunctionExpressionsInParens(expression: ts.Expression): ts.Expression { + return ts.transform(expression, [parensWrapperTransformerFactory]).transformed[0]; +} \ No newline at end of file diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index 8156c77e71..5cb2b5b395 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -60,9 +60,11 @@ runInEachFileSystem(() => { const refEmitter = new ReferenceEmitter([]); const handler = new ComponentDecoratorHandler( - reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry, false, - new NoopResourceLoader(), [''], false, true, '', moduleResolver, cycleAnalyzer, - refEmitter, NOOP_DEFAULT_IMPORT_RECORDER); + reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry, + /* isCore */ false, new NoopResourceLoader(), /* rootDirs */[''], + /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true, + /* i18nLegacyMessageIdFormat */ '', moduleResolver, cycleAnalyzer, refEmitter, + NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false); const TestCmp = getDeclaration(program, _('/entry.ts'), 'TestCmp', isNamedClassDeclaration); const detected = handler.detect(TestCmp, reflectionHost.getDecoratorsOfDeclaration(TestCmp)); if (detected === undefined) { diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts index 50d4efc594..5a0ccc47ce 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts @@ -49,7 +49,8 @@ runInEachFileSystem(() => { metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]), null); const handler = new DirectiveDecoratorHandler( - reflectionHost, evaluator, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, false); + reflectionHost, evaluator, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, + /* isCore */ false, /* annotateForClosureCompiler */ false); const analyzeDirective = (dirName: string) => { const DirNode = getDeclaration(program, _('/entry.ts'), dirName, isNamedClassDeclaration); diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts index 9619c00662..97901fd5c1 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts @@ -69,7 +69,8 @@ runInEachFileSystem(() => { const handler = new NgModuleDecoratorHandler( reflectionHost, evaluator, metaReader, metaRegistry, scopeRegistry, referencesRegistry, - false, null, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER); + false, null, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, + /* annotateForClosureCompiler */ false); const TestModule = getDeclaration(program, _('/entry.ts'), 'TestModule', isNamedClassDeclaration); const detected = diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 1ec33392c0..bbe9861762 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -604,16 +604,18 @@ export class NgtscProgram implements api.Program { this.isCore, this.resourceManager, this.rootDirs, this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false, this.getI18nLegacyMessageFormat(), this.moduleResolver, this.cycleAnalyzer, - this.refEmitter, this.defaultImportTracker, this.incrementalState), + this.refEmitter, this.defaultImportTracker, this.closureCompilerEnabled, + this.incrementalState), new DirectiveDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore), + this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore, + this.closureCompilerEnabled), new InjectableDecoratorHandler( this.reflector, this.defaultImportTracker, this.isCore, this.options.strictInjectionParameters || false), new NgModuleDecoratorHandler( this.reflector, evaluator, this.metaReader, metaRegistry, scopeRegistry, referencesRegistry, this.isCore, this.routeAnalyzer, this.refEmitter, - this.defaultImportTracker, this.options.i18nInLocale), + this.defaultImportTracker, this.closureCompilerEnabled, this.options.i18nInLocale), new PipeDecoratorHandler( this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore), ]; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 749ca50934..c4136b1a10 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -337,25 +337,170 @@ runInEachFileSystem(os => { // are valid for the real OS. When on non-Windows systems it doesn't like paths // that start with `C:`. if (os !== 'Windows' || platform() === 'win32') { - it('should add @nocollapse to static fields when closure annotations are requested', () => { - env.tsconfig({ - 'annotateForClosureCompiler': true, + describe('when closure annotations are requested', () => { + + it('should add @nocollapse to static fields', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write('test.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + templateUrl: './dir/test.html', + }) + export class TestCmp {} + `); + env.write('dir/test.html', '

Hello World

'); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp'); }); - env.write('test.ts', ` - import {Component} from '@angular/core'; - @Component({ - selector: 'test-cmp', - templateUrl: './dir/test.html', - }) - export class TestCmp {} - `); - env.write('dir/test.html', '

Hello World

'); + /** + * The following set of tests verify that after Tsickle run we do not have cases which + * trigger automatic semicolon insertion, which breaks the code. In order to avoid the + * problem, we wrap all function expressions in certain fields ("providers" and + * "viewProviders") in parentheses. More info on Tsickle processing related to this case can + * be found here: + * https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021 + */ + describe('wrap functions in certain fields in parentheses', () => { + const providers = ` + [{ + provide: 'token-a', + useFactory: (service: Service) => { + return () => service.id; + } + }, { + provide: 'token-b', + useFactory: function(service: Service) { + return function() { + return service.id; + } + } + }] + `; - env.driveMain(); + const service = ` + export class Service { + id: string = 'service-id'; + } + `; + + const verifyOutput = (jsContents: string) => { + // verify that there is no pattern that triggers automatic semicolon insertion + // by checking that there are no return statements not wrapped in parentheses + expect(trim(jsContents)).not.toContain(trim(` + return /** + * @return {?} + */ + `)); + expect(trim(jsContents)).toContain(trim(` + [{ + provide: 'token-a', + useFactory: (function (service) { + return (/** + * @return {?} + */ + function () { return service.id; }); + }) + }, { + provide: 'token-b', + useFactory: (function (service) { + return (/** + * @return {?} + */ + function () { + return service.id; + }); + }) + }] + `)); + }; + + it('should wrap functions in "providers" list in NgModule', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write('service.ts', service); + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + import {Service} from './service'; + + @NgModule({ + providers: ${providers} + }) + export class SomeModule {} + `); + + env.driveMain(); + verifyOutput(env.getContents('test.js')); + }); + + it('should wrap functions in "providers" list in Component', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write('service.ts', service); + env.write('test.ts', ` + import {Component} from '@angular/core'; + import {Service} from './service'; + + @Component({ + template: '...', + providers: ${providers} + }) + export class SomeComponent {} + `); + + env.driveMain(); + verifyOutput(env.getContents('test.js')); + }); + + it('should wrap functions in "viewProviders" list in Component', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write('service.ts', service); + env.write('test.ts', ` + import {Component} from '@angular/core'; + import {Service} from './service'; + + @Component({ + template: '...', + viewProviders: ${providers} + }) + export class SomeComponent {} + `); + + env.driveMain(); + verifyOutput(env.getContents('test.js')); + }); + + it('should wrap functions in "providers" list in Directive', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write('service.ts', service); + env.write('test.ts', ` + import {Directive} from '@angular/core'; + import {Service} from './service'; + + @Directive({ + providers: ${providers} + }) + export class SomeDirective {} + `); + + env.driveMain(); + verifyOutput(env.getContents('test.js')); + }); + }); - const jsContents = env.getContents('test.js'); - expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp'); }); }