From 80f4ff33381d37e72764d2ea3b7060e5952232e9 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 8 Feb 2021 13:32:04 -0800 Subject: [PATCH] fix(compiler-cli): set TS original node on imported namespace identifiers (#40711) This commit causes imports added by ngtsc's `ImportManager` to have their TypeScript "original node" set to the generated `ts.ImportDeclaration` statement. In g3, the tsickle transformer runs after the Angular transformer and post- processes Angular's compilation output. One of its post-processing tasks is to transform generated imports and references to imported symbols from the commonjs module system to the g3 module system. Part of this transformation involves recognizing modules with specific metadata and altering references to symbols from those modules accordingly. Normally, tsickle can rely on TypeScript's binding for an imported symbol to find its origin module and thus the correct metadata for the symbol. However the Angular transform generates new synthetic imports which don't have such binding information. Angular's imports are always namespace imports of the form: ``` import * as qualifier 'module/specifier'; ``` References to such an import are then of the form `qualifier.SymbolName`. To process such imports properly, tsickle needs to be able to associate the reference to `qualifier` in the expression `qualifer.SymbolName` with the `ts.ImportDeclaration` statement that defines it. It expects to do this by looking at the `ts.getOriginalNode()` for the `qualifier` reference, which should be the `ts.ImportDeclaration`. This commit changes ngtsc's import generation mechanism to set the original node on `qualifier` identifiers according to this expectation. This commit is not tested in the direct compiler tests, since: 1) there is no observable behavior externally from setting the original node 2) we don't have tests that intercept transformer operations (which could be used to directly assert against the AST nodes) 3) tsickle's published version does not (yet) contain the g3-specific transformations which rely on the original node and would thus allow the behavior to be observed. Instead, we rely on the g3 testing suite to validate the correctness of this fix. Breaking this functionality would cause g3 compilation errors for targets, since tsickle would be unable to transform imports correctly. PR Close #40711 --- .../rendering/commonjs_rendering_formatter.ts | 2 +- .../src/rendering/esm_rendering_formatter.ts | 2 +- .../src/rendering/umd_rendering_formatter.ts | 2 +- .../commonjs_rendering_formatter_spec.ts | 7 ++-- .../esm5_rendering_formatter_spec.ts | 7 ++-- .../rendering/esm_rendering_formatter_spec.ts | 7 ++-- .../ngcc/test/rendering/renderer_spec.ts | 4 +- .../rendering/umd_rendering_formatter_spec.ts | 38 ++++++++++--------- .../src/ngtsc/transform/src/utils.ts | 16 +++++++- .../src/ngtsc/translator/index.ts | 4 +- .../translator/src/api/import_generator.ts | 10 ----- .../ngtsc/translator/src/import_manager.ts | 25 +++++++++--- .../src/ngtsc/typecheck/src/context.ts | 2 +- .../ngtsc/typecheck/src/type_check_file.ts | 2 +- 14 files changed, 74 insertions(+), 54 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts index 426be4d4e7..f18a0181dc 100644 --- a/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts @@ -39,7 +39,7 @@ export class CommonJsRenderingFormatter extends Esm5RenderingFormatter { const insertionPoint = this.findEndOfImports(file); const renderedImports = - imports.map(i => `var ${i.qualifier} = require('${i.specifier}');\n`).join(''); + imports.map(i => `var ${i.qualifier.text} = require('${i.specifier}');\n`).join(''); output.appendLeft(insertionPoint, renderedImports); } diff --git a/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts index 17d55d5d45..7edf77e072 100644 --- a/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts @@ -42,7 +42,7 @@ export class EsmRenderingFormatter implements RenderingFormatter { const insertionPoint = this.findEndOfImports(sf); const renderedImports = - imports.map(i => `import * as ${i.qualifier} from '${i.specifier}';\n`).join(''); + imports.map(i => `import * as ${i.qualifier.text} from '${i.specifier}';\n`).join(''); output.appendLeft(insertionPoint, renderedImports); } diff --git a/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts b/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts index 4055c81865..662c046d07 100644 --- a/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts +++ b/packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts @@ -225,7 +225,7 @@ function renderFactoryParameters( } const parameters = factoryFunction.parameters; - const parameterString = imports.map(i => i.qualifier).join(','); + const parameterString = imports.map(i => i.qualifier.text).join(','); if (parameters.length > 0) { const injectionPoint = parameters[0].getFullStart(); output.appendLeft(injectionPoint, parameterString + ','); diff --git a/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts index 423a3ec673..6d3cf14b68 100644 --- a/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts @@ -177,8 +177,8 @@ exports.D = D; renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], sourceFile); expect(output.toString()).toContain(`/* A copyright notice */ @@ -257,7 +257,8 @@ var A = (function() {`); const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js')); const output = new MagicString(PROGRAM.contents); renderer.addConstants(output, 'var x = 3;', file); - renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file); + renderer.addImports( + output, [{specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}], file); expect(output.toString()).toContain(` var core = require('@angular/core'); var i0 = require('@angular/core'); diff --git a/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts index 4c82098f01..8da3f9323b 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts @@ -188,8 +188,8 @@ export { F }; renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], sourceFile); expect(output.toString()).toContain(`/* A copyright notice */ @@ -263,7 +263,8 @@ var A = (function() {`); const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js')); const output = new MagicString(PROGRAM.contents); renderer.addConstants(output, 'var x = 3;', file); - renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file); + renderer.addImports( + output, [{specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}], file); expect(output.toString()).toContain(` import {Directive} from '@angular/core'; import * as i0 from '@angular/core'; diff --git a/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts index eae5f15d9e..0f7749108e 100644 --- a/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts @@ -156,8 +156,8 @@ runInEachFileSystem(() => { renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], sourceFile); expect(output.toString()).toContain(`/* A copyright notice */ @@ -231,7 +231,8 @@ const x = 3; const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js')); const output = new MagicString(PROGRAM.contents); renderer.addConstants(output, 'const x = 3;', file); - renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file); + renderer.addImports( + output, [{specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}], file); expect(output.toString()).toContain(` import {Directive} from '@angular/core'; import * as i0 from '@angular/core'; diff --git a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts index 58654cebdf..e66474f3b6 100644 --- a/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts @@ -276,7 +276,7 @@ A.ɵcmp = ɵngcc0.ɵɵdefineComponent({ type: A, selectors: [["a"]], decls: 1, v const addImportsSpy = testFormatter.addImports as jasmine.Spy; expect(addImportsSpy.calls.first().args[0].toString()).toEqual(RENDERED_CONTENTS); expect(addImportsSpy.calls.first().args[1]).toEqual([ - {specifier: '@angular/core', qualifier: 'ɵngcc0'} + {specifier: '@angular/core', qualifier: jasmine.objectContaining({text: 'ɵngcc0'})} ]); }); @@ -693,7 +693,7 @@ UndecoratedBase.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, vie `function () { (typeof ngDevMode === "undefined" || ngDevMode) && ɵngcc0.setClassMetadata(`); const addImportsSpy = testFormatter.addImports as jasmine.Spy; expect(addImportsSpy.calls.first().args[1]).toEqual([ - {specifier: './r3_symbols', qualifier: 'ɵngcc0'} + {specifier: './r3_symbols', qualifier: jasmine.objectContaining({text: 'ɵngcc0'})} ]); }); diff --git a/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts b/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts index b1a6e2ca36..9872271c37 100644 --- a/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts +++ b/packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts @@ -200,8 +200,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); expect(output.toString()) @@ -217,8 +217,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); expect(output.toString()) @@ -233,8 +233,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); expect(output.toString()) @@ -249,10 +249,12 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@ngrx/store', qualifier: 'i0'}, - {specifier: '@angular/platform-browser-dynamic', qualifier: 'i1'}, - {specifier: '@angular/common/testing', qualifier: 'i2'}, - {specifier: '@angular-foo/package', qualifier: 'i3'} + {specifier: '@ngrx/store', qualifier: ts.createIdentifier('i0')}, { + specifier: '@angular/platform-browser-dynamic', + qualifier: ts.createIdentifier('i1') + }, + {specifier: '@angular/common/testing', qualifier: ts.createIdentifier('i2')}, + {specifier: '@angular-foo/package', qualifier: ts.createIdentifier('i3')} ], file); expect(output.toString()) @@ -270,8 +272,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); expect(output.toString()) @@ -287,8 +289,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); expect(output.toString()) @@ -315,8 +317,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); const outputSrc = output.toString(); @@ -361,8 +363,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib', renderer.addImports( output, [ - {specifier: '@angular/core', qualifier: 'i0'}, - {specifier: '@angular/common', qualifier: 'i1'} + {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}, + {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')} ], file); const outputSrc = output.toString(); diff --git a/packages/compiler-cli/src/ngtsc/transform/src/utils.ts b/packages/compiler-cli/src/ngtsc/transform/src/utils.ts index ddbdfe7397..57f08b9fa6 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/utils.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/utils.ts @@ -19,15 +19,27 @@ export function addImports( extraStatements: ts.Statement[] = []): ts.SourceFile { // Generate the import statements to prepend. const addedImports = importManager.getAllImports(sf.fileName).map(i => { - const qualifier = ts.createIdentifier(i.qualifier); + const qualifier = ts.createIdentifier(i.qualifier.text); const importClause = ts.createImportClause( /* name */ undefined, /* namedBindings */ ts.createNamespaceImport(qualifier)); - return ts.createImportDeclaration( + const decl = ts.createImportDeclaration( /* decorators */ undefined, /* modifiers */ undefined, /* importClause */ importClause, /* moduleSpecifier */ ts.createLiteral(i.specifier)); + + // Set the qualifier's original TS node to the `ts.ImportDeclaration`. This allows downstream + // transforms such as tsickle to properly process references to this import. + // + // This operation is load-bearing in g3 as some imported modules contain special metadata + // generated by clutz, which tsickle uses to transform imports and references to those imports. + // + // TODO(alxhub): add a test for this when tsickle is updated externally to depend on this + // behavior. + ts.setOriginalNode(i.qualifier, decl); + + return decl; }); // Filter out the existing imports and the source file body. All new statements diff --git a/packages/compiler-cli/src/ngtsc/translator/index.ts b/packages/compiler-cli/src/ngtsc/translator/index.ts index be45c20340..b6300ba02a 100644 --- a/packages/compiler-cli/src/ngtsc/translator/index.ts +++ b/packages/compiler-cli/src/ngtsc/translator/index.ts @@ -7,9 +7,9 @@ */ export {AstFactory, BinaryOperator, LeadingComment, ObjectLiteralProperty, SourceMapLocation, SourceMapRange, TemplateElement, TemplateLiteral, UnaryOperator, VariableDeclarationType} from './src/api/ast_factory'; -export {Import, ImportGenerator, NamedImport} from './src/api/import_generator'; +export {ImportGenerator, NamedImport} from './src/api/import_generator'; export {Context} from './src/context'; -export {ImportManager} from './src/import_manager'; +export {Import, ImportManager} from './src/import_manager'; export {ExpressionTranslatorVisitor, RecordWrappedNodeExprFn, TranslatorOptions} from './src/translator'; export {translateType} from './src/type_translator'; export {attachComments, createTemplateMiddle, createTemplateTail, TypeScriptAstFactory} from './src/typescript_ast_factory'; diff --git a/packages/compiler-cli/src/ngtsc/translator/src/api/import_generator.ts b/packages/compiler-cli/src/ngtsc/translator/src/api/import_generator.ts index 5e5fd3474d..07a1bd22f5 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/api/import_generator.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/api/import_generator.ts @@ -6,16 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -/** - * Information about an import that has been added to a module. - */ -export interface Import { - /** The name of the module that has been imported. */ - specifier: string; - /** The alias of the imported module. */ - qualifier: string; -} - /** * The symbol name and import namespace of an imported symbol, * which has been registered through the ImportGenerator. diff --git a/packages/compiler-cli/src/ngtsc/translator/src/import_manager.ts b/packages/compiler-cli/src/ngtsc/translator/src/import_manager.ts index f5eeec0f4b..65a083308e 100644 --- a/packages/compiler-cli/src/ngtsc/translator/src/import_manager.ts +++ b/packages/compiler-cli/src/ngtsc/translator/src/import_manager.ts @@ -7,7 +7,17 @@ */ import * as ts from 'typescript'; import {ImportRewriter, NoopImportRewriter} from '../../imports'; -import {Import, ImportGenerator, NamedImport} from './api/import_generator'; +import {ImportGenerator, NamedImport} from './api/import_generator'; + +/** + * Information about an import that has been added to a module. + */ +export interface Import { + /** The name of the module that has been imported. */ + specifier: string; + /** The `ts.Identifer` by which the imported module is known. */ + qualifier: ts.Identifier; +} export class ImportManager implements ImportGenerator { private specifierToIdentifier = new Map(); @@ -42,11 +52,14 @@ export class ImportManager implements ImportGenerator { } getAllImports(contextPath: string): Import[] { - const imports: {specifier: string, qualifier: string}[] = []; - this.specifierToIdentifier.forEach((qualifier, specifier) => { - specifier = this.rewriter.rewriteSpecifier(specifier, contextPath); - imports.push({specifier, qualifier: qualifier.text}); - }); + const imports: Import[] = []; + for (const [originalSpecifier, qualifier] of this.specifierToIdentifier) { + const specifier = this.rewriter.rewriteSpecifier(originalSpecifier, contextPath); + imports.push({ + specifier, + qualifier, + }); + } return imports; } } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts index e9729b1ff6..6754c93cc2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/context.ts @@ -355,7 +355,7 @@ export class TypeCheckContextImpl implements TypeCheckContext { // Write out the imports that need to be added to the beginning of the file. let imports = importManager.getAllImports(sf.fileName) - .map(i => `import * as ${i.qualifier} from '${i.specifier}';`) + .map(i => `import * as ${i.qualifier.text} from '${i.specifier}';`) .join('\n'); code = imports + '\n' + code; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts index 5b2996b8c4..88efdeb6b0 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts @@ -51,7 +51,7 @@ export class TypeCheckFile extends Environment { render(): string { let source: string = this.importManager.getAllImports(this.contextFile.fileName) - .map(i => `import * as ${i.qualifier} from '${i.specifier}';`) + .map(i => `import * as ${i.qualifier.text} from '${i.specifier}';`) .join('\n') + '\n\n'; const printer = ts.createPrinter();