From bab90a770917019cf138908a4f6a0e293065775e Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 19 Mar 2020 11:25:15 -0700 Subject: [PATCH] fix(compiler-cli): fix bug tracking indirect NgModule dependencies (#36211) The compiler needs to track the dependencies of a component, including any NgModules which happen to be present in a component's scope. If an upstream NgModule changes, any downstream components need to have their templates re-compiled and re-typechecked. Previously, the compiler handled this well for the A -> B -> C case where module A imports module B which re-exports module C. However, it fell apart in the A -> B -> C -> D case, because previously tracking focused on changes to components/directives in the scope, and not NgModules specifically. This commit introduces logic to track which NgModules contributed to a given scope, and treat them as dependencies of any components within. This logic also contains a bug, which is intentional for now. It purposefully does not track transitive dependencies of the NgModules which contribute to a scope. If it did, using the current dependency system, this would treat all components and directives (even those not exported into the scope) as dependencies, causing a major performance bottleneck. Only those dependencies which contributed to the module's export scope should be considered, but the current system is incapable of making this distinction. This will be fixed at a later date. PR Close #36211 --- .../src/ngtsc/core/src/compiler.ts | 23 ++++++ .../compiler-cli/src/ngtsc/scope/src/api.ts | 6 ++ .../src/ngtsc/scope/src/dependency.ts | 10 ++- .../compiler-cli/src/ngtsc/scope/src/local.ts | 13 ++++ .../test/ngtsc/incremental_spec.ts | 78 +++++++++++++++++++ 5 files changed, 129 insertions(+), 1 deletion(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 91edb09bb0..d0dc4062c1 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -541,6 +541,29 @@ export class NgCompiler { // pipe's name may have changed. depGraph.addTransitiveDependency(file, pipe.ref.node.getSourceFile()); } + + // Components depend on the entire export scope. In addition to transitive dependencies on + // all directives/pipes in the export scope, they also depend on every NgModule in the + // scope, as changes to a module may add new directives/pipes to the scope. + for (const depModule of scope.ngModules) { + // There is a correctness issue here. To be correct, this should be a transitive + // dependency on the depModule file, since the depModule's exports might change via one of + // its dependencies, even if depModule's file itself doesn't change. However, doing this + // would also trigger recompilation if a non-exported component or directive changed, + // which causes performance issues for rebuilds. + // + // Given the rebuild issue is an edge case, currently we err on the side of performance + // instead of correctness. A correct and performant design would distinguish between + // changes to the depModule which affect its export scope and changes which do not, and + // only add a dependency for the former. This concept is currently in development. + // + // TODO(alxhub): fix correctness issue by understanding the semantics of the dependency. + depGraph.addDependency(file, depModule.getSourceFile()); + } + } else { + // Directives (not components) and pipes only depend on the NgModule which directly declares + // them. + depGraph.addDependency(file, ngModuleFile); } } this.perfRecorder.stop(recordSpan); diff --git a/packages/compiler-cli/src/ngtsc/scope/src/api.ts b/packages/compiler-cli/src/ngtsc/scope/src/api.ts index 774277345e..64fd1fb5e9 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/api.ts @@ -7,6 +7,7 @@ */ import {DirectiveMeta, PipeMeta} from '../../metadata'; +import {ClassDeclaration} from '../../reflection'; /** @@ -22,6 +23,11 @@ export interface ScopeData { * Pipes in the exported scope of the module. */ pipes: PipeMeta[]; + + /** + * NgModules which contributed to the scope of the module. + */ + ngModules: ClassDeclaration[]; } /** diff --git a/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts b/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts index 94479f97b9..afe0354d77 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/dependency.ts @@ -58,6 +58,7 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver { // Build up the export scope - those directives and pipes made visible by this module. const directives: DirectiveMeta[] = []; const pipes: PipeMeta[] = []; + const ngModules = new Set([clazz]); const meta = this.dtsMetaReader.getNgModuleMetadata(ref); if (meta === null) { @@ -114,6 +115,9 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver { for (const pipe of exportScope.exported.pipes) { pipes.push(this.maybeAlias(pipe, sourceFile, /* isReExport */ true)); } + for (const ngModule of exportScope.exported.ngModules) { + ngModules.add(ngModule); + } } } continue; @@ -124,7 +128,11 @@ export class MetadataDtsModuleScopeResolver implements DtsModuleScopeResolver { } const exportScope: ExportScope = { - exported: {directives, pipes}, + exported: { + directives, + pipes, + ngModules: Array.from(ngModules), + }, }; this.cache.set(clazz, exportScope); return exportScope; diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 5cc992b4e9..e4427c7e50 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -278,6 +278,11 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop return null; } + // Modules which contributed to the compilation scope of this module. + const compilationModules = new Set([ngModule.ref.node]); + // Modules which contributed to the export scope of this module. + const exportedModules = new Set([ngModule.ref.node]); + // Errors produced during computation of the scope are recorded here. At the end, if this array // isn't empty then `undefined` will be cached and returned to indicate this scope is invalid. const diagnostics: ts.Diagnostic[] = []; @@ -329,6 +334,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop for (const pipe of importScope.exported.pipes) { compilationPipes.set(pipe.ref.node, pipe); } + for (const importedModule of importScope.exported.ngModules) { + compilationModules.add(importedModule); + } } // 2) add declarations. @@ -379,6 +387,9 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop for (const pipe of importScope.exported.pipes) { exportPipes.set(pipe.ref.node, pipe); } + for (const exportedModule of importScope.exported.ngModules) { + exportedModules.add(exportedModule); + } } else if (compilationDirectives.has(decl.node)) { // decl is a directive or component in the compilation scope of this NgModule. const directive = compilationDirectives.get(decl.node)!; @@ -402,6 +413,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop const exported = { directives: Array.from(exportDirectives.values()), pipes: Array.from(exportPipes.values()), + ngModules: Array.from(exportedModules), }; const reexports = this.getReexports(ngModule, ref, declared, exported, diagnostics); @@ -424,6 +436,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop compilation: { directives: Array.from(compilationDirectives.values()), pipes: Array.from(compilationPipes.values()), + ngModules: Array.from(compilationModules), }, exported, reexports, diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index 6cf7f99343..a3c97fac04 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -251,6 +251,84 @@ runInEachFileSystem(() => { expect(written).toContain('/foo_module.js'); }); + it('should rebuild a component if one of its deep NgModule dependencies changes', () => { + // This test constructs a chain of NgModules: + // - EntryModule imports MiddleAModule + // - MiddleAModule exports MiddleBModule + // - MiddleBModule exports DirModule + // The last link (MiddleBModule exports DirModule) is not present initially, but is added + // during a recompilation. + // + // Since the dependency from EntryModule on the contents of MiddleBModule is not "direct" + // (meaning MiddleBModule is not discovered during analysis of EntryModule), this test is + // verifying that NgModule dependency tracking correctly accounts for this situation. + env.write('entry.ts', ` + import {Component, NgModule} from '@angular/core'; + import {MiddleAModule} from './middle-a'; + + @Component({ + selector: 'test-cmp', + template: '
', + }) + export class TestCmp {} + + @NgModule({ + declarations: [TestCmp], + imports: [MiddleAModule], + }) + export class EntryModule {} + `); + env.write('middle-a.ts', ` + import {NgModule} from '@angular/core'; + import {MiddleBModule} from './middle-b'; + + @NgModule({ + exports: [MiddleBModule], + }) + export class MiddleAModule {} + `); + env.write('middle-b.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({}) + export class MiddleBModule {} + `); + env.write('dir_module.ts', ` + import {NgModule} from '@angular/core'; + import {Dir} from './dir'; + + @NgModule({ + declarations: [Dir], + exports: [Dir], + }) + export class DirModule {} + `); + env.write('dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({ + selector: '[dir]', + }) + export class Dir {} + `); + + env.driveMain(); + expect(env.getContents('entry.js')).not.toContain('Dir'); + + env.write('middle-b.ts', ` + import {NgModule} from '@angular/core'; + import {DirModule} from './dir_module'; + + @NgModule({ + exports: [DirModule], + }) + export class MiddleBModule {} + `); + + env.driveMain(); + expect(env.getContents('entry.js')).toContain('Dir'); + }); + it('should rebuild a component if removed from an NgModule', () => { // This test consists of a component with a dependency (the directive DepDir) provided via an // NgModule. Initially this configuration is built, then the component is removed from its