From 91ce543ef4d101d11b17c6b584325d9f80435560 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 17 May 2022 14:43:03 -0700 Subject: [PATCH] fix(compiler-cli): handle standalone components with cycles (#46029) The Angular compiler performs cycle detection when generating imports within component files. This was previously necessary as reifying dependencies discovered via NgModules into the component output could add imports that weren't present in the original component and potentially create cycles. Doing this could cause order-of-execution issues with existing user imports, so the compiler detects this case and falls back to an alternative way of specifying component dependencies that doesn't risk creating cycles. For standalone components, Angular does not need to add new imports to the component file as the user has already explicitly referenced dependencies in the `@Component.imports`. As a result, the cycle detection can be skipped. Correctly authoring a program with import cycles is always challenging. One side of a cyclic import will always initially evaluate to `undefined`, and this can result in errors in the component definition when this happens within component `imports`. Our compiler _could_ detect the cycle and choose to wrap the component dependencies in an automatic closure instead, avoiding any issues with `undefined` during an eager evaluation. However, this commit makes an active choice not to do that as it only serves to mask the problems with cyclic imports. Future refactorings may cause the "other half" of the cycle to break. Users should instead be aware of the potential problems with cycles and explicitly defer evaluations with `forwardRef` where needed. This ensures that future implementations of Angular compilation which may not be able to automatically detect import cycles and correct accordingly can still compile such components. PR Close #46029 --- .../annotations/component/src/handler.ts | 34 ++++++++++------- .../test/ngtsc/standalone_spec.ts | 38 +++++++++++++++++++ 2 files changed, 58 insertions(+), 14 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts index 7f608c592a2..1a71661c351 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -717,20 +717,26 @@ export class ComponentDecoratorHandler implements symbol.usedPipes = declarations.filter(isUsedPipe).map(getSemanticReference); } - // Scan through the directives/pipes actually used in the template and check whether any - // import which needs to be generated would create a cycle. const cyclesFromDirectives = new Map(); const cyclesFromPipes = new Map(); - for (const usedDep of declarations) { - const cycle = this._checkForCyclicImport(usedDep.importedFile, usedDep.type, context); - if (cycle !== null) { - switch (usedDep.kind) { - case R3TemplateDependencyKind.Directive: - cyclesFromDirectives.set(usedDep, cycle); - break; - case R3TemplateDependencyKind.Pipe: - cyclesFromPipes.set(usedDep, cycle); - break; + + // Scan through the directives/pipes actually used in the template and check whether any + // import which needs to be generated would create a cycle. This check is skipped for + // standalone components as the dependencies of a standalone component have already been + // imported directly by the user, so Angular won't introduce any imports that aren't already + // in the user's program. + if (!metadata.isStandalone) { + for (const usedDep of declarations) { + const cycle = this._checkForCyclicImport(usedDep.importedFile, usedDep.type, context); + if (cycle !== null) { + switch (usedDep.kind) { + case R3TemplateDependencyKind.Directive: + cyclesFromDirectives.set(usedDep, cycle); + break; + case R3TemplateDependencyKind.Pipe: + cyclesFromPipes.set(usedDep, cycle); + break; + } } } } @@ -740,7 +746,7 @@ export class ComponentDecoratorHandler implements // No cycle was detected. Record the imports that need to be created in the cycle detector // so that future cyclic import checks consider their production. for (const {type, importedFile} of declarations) { - this._recordSyntheticImport(importedFile, type, context); + this.maybeRecordSyntheticImport(importedFile, type, context); } // Check whether the dependencies arrays in ɵcmp need to be wrapped in a closure. @@ -932,7 +938,7 @@ export class ComponentDecoratorHandler implements return this.cycleAnalyzer.wouldCreateCycle(origin, imported); } - private _recordSyntheticImport( + private maybeRecordSyntheticImport( importedFile: ImportedFile, expr: Expression, origin: ts.SourceFile): void { const imported = resolveImportedFile(this.moduleResolver, importedFile, expr, origin); if (imported === null) { diff --git a/packages/compiler-cli/test/ngtsc/standalone_spec.ts b/packages/compiler-cli/test/ngtsc/standalone_spec.ts index 9c5b743db3c..b87e42c3baa 100644 --- a/packages/compiler-cli/test/ngtsc/standalone_spec.ts +++ b/packages/compiler-cli/test/ngtsc/standalone_spec.ts @@ -93,6 +93,44 @@ runInEachFileSystem(() => { expect(dtsCode).toContain('i0.ɵɵPipeDeclaration'); }); + it('should compile a standalone component even in the presence of cycles', () => { + env.write('dep.ts', ` + import {Directive, Input} from '@angular/core'; + + // This import creates a cycle, since 'test.ts' imports 'dir.ts'. + import {TestType} from './test'; + + @Directive({ + standalone: true, + selector: '[dir]', + }) + export class TestDir { + @Input() value?: TestType; + } + `); + env.write('test.ts', ` + import {Component} from '@angular/core'; + import {TestDir} from './dep'; + + export interface TestType { + value: string; + } + + @Component({ + standalone: true, + imports: [TestDir], + selector: 'test-cmp', + template: '
', + }) + export class TestCmp {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('dependencies: [i1.TestDir]'); + }); + it('should error when a non-standalone component tries to use imports', () => { env.write('test.ts', ` import {Component, Directive} from '@angular/core';