From be121bba853b7f0b6ef8e88ddcffdbf075345e8f Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Wed, 13 Feb 2019 18:13:29 -0800 Subject: [PATCH] fix(ivy): restore @fileoverview annotations for Closure (#28723) Prior to this change, the @fileoverview annotations added by users in source files or by tsickle during compilation might have change a location due to the fact that Ngtsc may prepend extra imports or constants. As a result, the output file is considered invalid by Closure (misplaced @fileoverview annotation). In order to resolve the problem we relocate @fileoverview annotation if we detect that its host node shifted. PR Close #28723 --- packages/compiler-cli/src/ngtsc/program.ts | 5 +- .../src/ngtsc/transform/src/transform.ts | 67 +++++++++++- .../src/ngtsc/transform/src/utils.ts | 8 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 103 ++++++++++++++++++ 4 files changed, 174 insertions(+), 9 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index 620be30788..ed44cdb0a7 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -272,8 +272,9 @@ export class NgtscProgram implements api.Program { }; const customTransforms = opts && opts.customTransformers; - const beforeTransforms = - [ivyTransformFactory(compilation, this.reflector, this.importRewriter, this.isCore)]; + const beforeTransforms = [ivyTransformFactory( + compilation, this.reflector, this.importRewriter, this.isCore, + this.closureCompilerEnabled)]; const afterDeclarationsTransforms = [declarationTransformFactory(compilation)]; if (this.factoryToSourceInfo !== null) { diff --git a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts index 5223ee376a..89fb187f1c 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts @@ -20,12 +20,24 @@ import {addImports} from './utils'; const NO_DECORATORS = new Set(); +const CLOSURE_FILE_OVERVIEW_REGEXP = /\s+@fileoverview\s+/i; + +/** + * Metadata to support @fileoverview blocks (Closure annotations) extracting/restoring. + */ +interface FileOverviewMeta { + comments: ts.SynthesizedComment[]; + host: ts.Statement; + trailing: boolean; +} + export function ivyTransformFactory( compilation: IvyCompilation, reflector: ReflectionHost, importRewriter: ImportRewriter, - isCore: boolean): ts.TransformerFactory { + isCore: boolean, isClosureCompilerEnabled: boolean): ts.TransformerFactory { return (context: ts.TransformationContext): ts.Transformer => { return (file: ts.SourceFile): ts.SourceFile => { - return transformIvySourceFile(compilation, context, reflector, importRewriter, isCore, file); + return transformIvySourceFile( + compilation, context, reflector, importRewriter, file, isCore, isClosureCompilerEnabled); }; }; } @@ -189,20 +201,65 @@ class IvyVisitor extends Visitor { */ function transformIvySourceFile( compilation: IvyCompilation, context: ts.TransformationContext, reflector: ReflectionHost, - importRewriter: ImportRewriter, isCore: boolean, file: ts.SourceFile): ts.SourceFile { + importRewriter: ImportRewriter, file: ts.SourceFile, isCore: boolean, + isClosureCompilerEnabled: boolean): ts.SourceFile { const constantPool = new ConstantPool(); const importManager = new ImportManager(importRewriter); // Recursively scan through the AST and perform any updates requested by the IvyCompilation. const visitor = new IvyVisitor(compilation, reflector, importManager, isCore, constantPool); - const sf = visit(file, visitor, context); + let sf = visit(file, visitor, context); // Generate the constant statements first, as they may involve adding additional imports // to the ImportManager. const constants = constantPool.statements.map(stmt => translateStatement(stmt, importManager)); + // Preserve @fileoverview comments required by Closure, since the location might change as a + // result of adding extra imports and constant pool statements. + const fileOverviewMeta = isClosureCompilerEnabled ? getFileOverviewComment(sf.statements) : null; + // Add new imports for this file. - return addImports(importManager, sf, constants); + sf = addImports(importManager, sf, constants); + + if (fileOverviewMeta !== null) { + setFileOverviewComment(sf, fileOverviewMeta); + } + + return sf; +} + +function getFileOverviewComment(statements: ts.NodeArray): FileOverviewMeta|null { + if (statements.length > 0) { + const host = statements[0]; + let trailing = false; + let comments = ts.getSyntheticLeadingComments(host); + // If @fileoverview tag is not found in source file, tsickle produces fake node with trailing + // comment and inject it at the very beginning of the generated file. So we need to check for + // leading as well as trailing comments. + if (!comments || comments.length === 0) { + trailing = true; + comments = ts.getSyntheticTrailingComments(host); + } + if (comments && comments.length > 0 && CLOSURE_FILE_OVERVIEW_REGEXP.test(comments[0].text)) { + return {comments, host, trailing}; + } + } + return null; +} + +function setFileOverviewComment(sf: ts.SourceFile, fileoverview: FileOverviewMeta): void { + const {comments, host, trailing} = fileoverview; + // If host statement is no longer the first one, it means that extra statements were added at the + // very beginning, so we need to relocate @fileoverview comment and cleanup the original statement + // that hosted it. + if (sf.statements.length > 0 && host !== sf.statements[0]) { + if (trailing) { + ts.setSyntheticTrailingComments(host, undefined); + } else { + ts.setSyntheticLeadingComments(host, undefined); + } + ts.setSyntheticLeadingComments(sf.statements[0], comments); + } } function maybeFilterDecorator( diff --git a/packages/compiler-cli/src/ngtsc/transform/src/utils.ts b/packages/compiler-cli/src/ngtsc/transform/src/utils.ts index 03158c34ea..9e6df27e95 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/utils.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/utils.ts @@ -31,8 +31,12 @@ export function addImports( const body = sf.statements.filter(stmt => !isImportStatement(stmt)); // Prepend imports if needed. if (addedImports.length > 0) { - sf.statements = - ts.createNodeArray([...existingImports, ...addedImports, ...extraStatements, ...body]); + // If we prepend imports, we also prepend NotEmittedStatement to use it as an anchor + // for @fileoverview Closure annotation. If there is no @fileoverview annotations, this + // statement would be a noop. + const fileoverviewAnchorStmt = ts.createNotEmittedStatement(sf); + sf.statements = ts.createNodeArray( + [fileoverviewAnchorStmt, ...existingImports, ...addedImports, ...extraStatements, ...body]); } return sf; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index a27662e909..bc28e284d7 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -2179,6 +2179,109 @@ describe('ngtsc behavioral tests', () => { expect(afterCount).toBe(1); }); + describe('@fileoverview Closure annotations', () => { + it('should be produced if not present in source file', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write(`test.ts`, ` + import {Component} from '@angular/core'; + + @Component({ + template: '
', + }) + export class SomeComp {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const fileoverview = ` + /** + * @fileoverview added by tsickle + * @suppress {checkTypes,extraRequire,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc + */ + `; + expect(trim(jsContents).startsWith(trim(fileoverview))).toBeTruthy(); + }); + + it('should be produced for empty source files', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write(`test.ts`, ``); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const fileoverview = ` + /** + * @fileoverview added by tsickle + * @suppress {checkTypes,extraRequire,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc + */ + `; + expect(trim(jsContents).startsWith(trim(fileoverview))).toBeTruthy(); + }); + + it('should always be at the very beginning of a script (if placed above imports)', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write(`test.ts`, ` + /** + * @fileoverview Some Comp overview + * @modName {some_comp} + */ + + import {Component} from '@angular/core'; + + @Component({ + template: '
', + }) + export class SomeComp {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const fileoverview = ` + /** + * + * @fileoverview Some Comp overview + * @modName {some_comp} + * + * @suppress {checkTypes,extraRequire,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc + */ + `; + expect(trim(jsContents).startsWith(trim(fileoverview))).toBeTruthy(); + }); + + it('should always be at the very beginning of a script (if placed above non-imports)', () => { + env.tsconfig({ + 'annotateForClosureCompiler': true, + }); + env.write(`test.ts`, ` + /** + * @fileoverview Some Comp overview + * @modName {some_comp} + */ + + const testConst = 'testConstValue'; + const testFn = function() { return true; } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + const fileoverview = ` + /** + * + * @fileoverview Some Comp overview + * @modName {some_comp} + * + * @suppress {checkTypes,extraRequire,missingReturn,unusedPrivateMembers,uselessCode} checked by tsc + */ + `; + expect(trim(jsContents).startsWith(trim(fileoverview))).toBeTruthy(); + }); + }); + describe('sanitization', () => { it('should generate sanitizers for unsafe attributes in hostBindings fn in Directives', () => { env.tsconfig();