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