From 7f1651574ee95eaaa5f636fc37be7b07d1a808e5 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 10 Apr 2021 21:45:59 +0200 Subject: [PATCH] fix(compiler-cli): prevent eliding default imports in incremental recompilations (#41557) The Angular compiler has to actively keep default import statements alive if they were only used in type-only positions, but have been emitted as value expressions for DI purposes. A problem occurred in incremental recompilations, where the relationship between an identifier usage and its corresponding default import would not be considered. This could result in the removal of the default import statement and caused a `ReferenceError` at runtime. This commit fixes the issue by storing the association from an identifier to its default import declaration on the source file itself, instead of within the `DefaultImportTracker` instance. The `DefaultImportTracker` instance is only valid for a single compilation, whereas the association from an identifier to a default import declaration is valid as long as the `ts.SourceFile` is the same instance. A subsequent commit refactor the `DefaultImportTracker` to no longer be responsible for registering the association, as its lifetime is conceptually too short to do so. Fixes #41377 PR Close #41557 --- .../src/ngtsc/imports/src/default.ts | 32 +++++----- .../test/ngtsc/incremental_spec.ts | 61 +++++++++++++++++++ 2 files changed, 76 insertions(+), 17 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/imports/src/default.ts b/packages/compiler-cli/src/ngtsc/imports/src/default.ts index ca6bf80125..23ff5be771 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/default.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/default.ts @@ -48,6 +48,12 @@ export const NOOP_DEFAULT_IMPORT_RECORDER: DefaultImportRecorder = { recordUsedIdentifier: (id: ts.Identifier) => void{}, }; +const ImportDeclarationMapping = Symbol('ImportDeclarationMapping'); + +interface SourceFileWithImportDeclarationMapping extends ts.SourceFile { + [ImportDeclarationMapping]?: Map; +} + /** * TypeScript has trouble with generating default imports inside of transformers for some module * formats. The issue is that for the statement: @@ -79,38 +85,31 @@ export const NOOP_DEFAULT_IMPORT_RECORDER: DefaultImportRecorder = { * "import * as X" style imports for those, and the "X" identifier survives transformation. */ export class DefaultImportTracker implements DefaultImportRecorder { - /** - * A `Map` which tracks the `Map` of default import `ts.Identifier`s to their - * `ts.ImportDeclaration`s. These declarations are not guaranteed to be used. - */ - private sourceFileToImportMap = - new Map>(); - /** * A `Map` which tracks the `Set` of `ts.ImportDeclaration`s for default imports that were used in * a given `ts.SourceFile` and need to be preserved. */ private sourceFileToUsedImports = new Map>(); recordImportedIdentifier(id: ts.Identifier, decl: ts.ImportDeclaration): void { - const sf = getSourceFile(id); - if (!this.sourceFileToImportMap.has(sf)) { - this.sourceFileToImportMap.set(sf, new Map()); + const sf = getSourceFile(id) as SourceFileWithImportDeclarationMapping; + if (sf[ImportDeclarationMapping] === undefined) { + sf[ImportDeclarationMapping] = new Map(); } - this.sourceFileToImportMap.get(sf)!.set(id, decl); + sf[ImportDeclarationMapping]!.set(id, decl); } recordUsedIdentifier(id: ts.Identifier): void { - const sf = getSourceFile(id); - if (!this.sourceFileToImportMap.has(sf)) { + const sf = getSourceFile(id) as SourceFileWithImportDeclarationMapping; + const identifierToDeclaration = sf[ImportDeclarationMapping]; + if (identifierToDeclaration === undefined) { // The identifier's source file has no registered default imports at all. return; } - const identiferToDeclaration = this.sourceFileToImportMap.get(sf)!; - if (!identiferToDeclaration.has(id)) { + if (!identifierToDeclaration.has(id)) { // The identifier isn't from a registered default import. return; } - const decl = identiferToDeclaration.get(id)!; + const decl = identifierToDeclaration.get(id)!; // Add the default import declaration to the set of used import declarations for the file. if (!this.sourceFileToUsedImports.has(sf)) { @@ -182,7 +181,6 @@ export class DefaultImportTracker implements DefaultImportRecorder { // Save memory - there's no need to keep these around once the transform has run for the given // file. - this.sourceFileToImportMap.delete(originalSf); this.sourceFileToUsedImports.delete(originalSf); return ts.updateSourceFileNode(sf, statements); diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index 06c65d53cb..5b94f8ccc8 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -605,6 +605,67 @@ runInEachFileSystem(() => { expect(env.driveDiagnostics().length).toBe(1); }); + it('should retain default imports that have been converted into a value expression', () => { + // This test defines the component `TestCmp` that has a default-imported class as + // constructor parameter, and uses `TestDir` in its template. An incremental compilation + // updates `TestDir` and changes its inputs, thereby triggering re-emit of `TestCmp` without + // performing re-analysis of `TestCmp`. The output of the re-emitted file for `TestCmp` + // should continue to have retained the default import. + env.write('service.ts', ` + import {Injectable} from '@angular/core'; + + @Injectable({ providedIn: 'root' }) + export default class DefaultService {} + `); + env.write('cmp.ts', ` + import {Component, Directive} from '@angular/core'; + import DefaultService from './service'; + + @Component({ + template: '
', + }) + export class TestCmp { + constructor(service: DefaultService) {} + } + `); + env.write('dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({ selector: '[dir]' }) + export class TestDir {} + `); + env.write('mod.ts', ` + import {NgModule} from '@angular/core'; + import {TestDir} from './dir'; + import {TestCmp} from './cmp'; + + @NgModule({ declarations: [TestDir, TestCmp] }) + export class TestMod {} + `); + + env.driveMain(); + env.flushWrittenFileTracking(); + + // Update `TestDir` to change its inputs, triggering a re-emit of `TestCmp` that uses + // `TestDir`. + env.write('dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({ selector: '[dir]', inputs: ['added'] }) + export class TestDir {} + `); + env.driveMain(); + + // Verify that `TestCmp` was indeed re-emitted. + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).toContain('/dir.js'); + expect(written).toContain('/cmp.js'); + + // Verify that the default import is still present. + const content = env.getContents('cmp.js'); + expect(content).toContain(`import DefaultService from './service';`); + }); + it('should recompile when a remote change happens to a scope', () => { // The premise of this test is that the component Cmp has a template error (a binding to an // unknown property). Cmp is in ModuleA, which imports ModuleB, which declares Dir that has