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

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);

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[];
 }
 
 /**

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<ClassDeclaration>([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;

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<ClassDeclaration>([ngModule.ref.node]);
+    // Modules which contributed to the export scope of this module.
+    const exportedModules = new Set<ClassDeclaration>([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,

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: '<div dir>',
+        })
+        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