From 65bccb07feed1da5e393429c1260565a5819c073 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 5 Nov 2019 18:00:55 -0800 Subject: [PATCH 1/2] fix(ivy): wrap functions from "providers" in parentheses in Closure mode Due to the fact that Tsickle runs between analyze and transform phases in Angular, Tsickle may transform nodes (add comments with type annotations for Closure) that we captured during the analyze phase. As a result, some patterns where a function is returned from another function may trigger automatic semicolon insertion, which breaks the code (makes functions return `undefined` instead of a function). In order to avoid the problem, this commit updates the code to wrap all functions in some expression ("privders" and "viewProviders") in parentheses. More info can be found in Tsickle source code here: https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021 --- .../ngcc/src/analysis/decoration_analyzer.ts | 7 +- .../src/ngtsc/annotations/src/component.ts | 14 +- .../src/ngtsc/annotations/src/directive.ts | 20 ++- .../src/ngtsc/annotations/src/metadata.ts | 24 +-- .../src/ngtsc/annotations/src/ng_module.ts | 14 +- .../src/ngtsc/annotations/src/util.ts | 25 ++++ .../ngtsc/annotations/test/component_spec.ts | 8 +- .../ngtsc/annotations/test/directive_spec.ts | 3 +- .../ngtsc/annotations/test/ng_module_spec.ts | 3 +- packages/compiler-cli/src/ngtsc/program.ts | 8 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 141 ++++++++++++++++++ 11 files changed, 232 insertions(+), 35 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 5bf87431e614f..258b94b473da5 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 560f5dde95c9d..7706e298743cd 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 6a918a9b21947..38688f5ba87d8 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 9338c056de623..d5227f9cff35a 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; } @@ -109,8 +111,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}); } @@ -122,8 +124,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); } @@ -131,7 +133,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.'); } @@ -141,7 +144,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 8df4671232e5f..3268872c30e74 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 = (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 8156c77e71380..5cb2b5b395c02 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 50d4efc594966..5a0ccc47ce65d 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 9619c00662c49..97901fd5c1a44 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 96e571e81e781..1f17e7917574f 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -591,16 +591,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 b835975d416b7..d1fbb378d2ccc 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -357,6 +357,147 @@ runInEachFileSystem(os => { const jsContents = env.getContents('test.js'); expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp'); }); + + /** + * 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 when closure annotations are requested', + () => { + const providers = ` + [{ + provide: 'token-a', + useFactory: (service: Service) => { + return () => service.id; + } + }, { + provide: 'token-b', + useFactory: function(service: Service) { + return function() { + return service.id; + } + } + }] + `; + + const service = ` + export class Service { + id: string = 'service-id'; + } + `; + + const verifyOutput = (jsContents: string) => { + // verify that there is no pattern that triggers automatic semicolon insertion + 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')); + }); + }); } it('should recognize aliased decorators', () => { From 4de646c1f066dab39fd16a6e9697a17ff6f75405 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Wed, 13 Nov 2019 16:49:46 -0800 Subject: [PATCH 2/2] fixup! fix(ivy): wrap functions from "providers" in parentheses in Closure mode --- .../src/ngtsc/annotations/src/util.ts | 2 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 288 +++++++++--------- 2 files changed, 147 insertions(+), 143 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index ceccbab01d747..37be1de1c146e 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -354,7 +354,7 @@ export function readBaseClass( const parensWrapperTransformerFactory: ts.TransformerFactory = (context: ts.TransformationContext) => { - const visitor = (node: ts.Node): ts.Node => { + 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); diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index d1fbb378d2ccc..fa8439cc5c4a5 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -337,167 +337,171 @@ 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, - }); - env.write('test.ts', ` - import {Component} from '@angular/core'; + describe('when closure annotations are requested', () => { - @Component({ - selector: 'test-cmp', - templateUrl: './dir/test.html', - }) - export class TestCmp {} - `); - env.write('dir/test.html', '

Hello World

'); + it('should add @nocollapse to static fields', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write('test.ts', ` + import {Component} from '@angular/core'; - env.driveMain(); + @Component({ + selector: 'test-cmp', + templateUrl: './dir/test.html', + }) + export class TestCmp {} + `); + env.write('dir/test.html', '

Hello World

'); - const jsContents = env.getContents('test.js'); - expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp'); - }); + env.driveMain(); - /** - * 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 when closure annotations are requested', - () => { - const providers = ` - [{ - provide: 'token-a', - useFactory: (service: Service) => { - return () => service.id; - } - }, { - provide: 'token-b', - useFactory: function(service: Service) { - return function() { - return service.id; - } + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp'); + }); + + /** + * 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; } - }] - `; + } + }] + `; - const service = ` + const service = ` export class Service { id: string = 'service-id'; } `; - const verifyOutput = (jsContents: string) => { - // verify that there is no pattern that triggers automatic semicolon insertion - 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')); + 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 {} + `); - 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')); + 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 "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')); + 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')); }); + }); + + }); } it('should recognize aliased decorators', () => {