From 5cd26a94206dfe8aabdf0dd15bfc09e7a8c606da Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 12 Feb 2025 15:13:17 +0100 Subject: [PATCH] fix(compiler-cli): handle deferred blocks with shared dependencies correctly (#59926) When the compiler analyzes the defer blocks in a component, it generates two sets of dependencies: ones specific for each block and others from all the deferred blocks within the component. The logic that combines all the defer block dependencies wasn't de-duplicating them which resulted in us producing `setClassMetadataAsync` calls where the callback can have multiple parameters with the same name. This was a problem both in full and partial compilation, but the latter was more visible, because Babel throws an error in such cases. These changes add some logic to de-duplicate the dependencies so that we produce valid code. Fixes #59922. PR Close #59926 --- .../annotations/component/src/handler.ts | 16 +++- .../GOLDEN_PARTIAL.js | 96 +++++++++++++++++++ .../r3_view_compiler_deferred/TEST_CASES.json | 19 ++++ .../deferred_with_duplicate_external_dep.ts | 21 ++++ ...ferred_with_duplicate_external_dep_lazy.ts | 4 + ...erred_with_duplicate_external_dep_other.ts | 4 + ...ed_with_duplicate_external_dep_template.js | 43 +++++++++ 7 files changed, 200 insertions(+), 3 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_lazy.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_other.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_template.js 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 d207b5288f7..c7e12284542 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -1888,22 +1888,32 @@ export class ComponentDecoratorHandler private resolveAllDeferredDependencies( resolution: Readonly, ): R3DeferPerComponentDependency[] { + const seenDeps = new Set(); const deferrableTypes: R3DeferPerComponentDependency[] = []; // Go over all dependencies of all defer blocks and update the value of // the `isDeferrable` flag and the `importPath` to reflect the current // state after visiting all components during the `resolve` phase. for (const [_, deps] of resolution.deferPerBlockDependencies) { for (const deferBlockDep of deps) { - const importDecl = - resolution.deferrableDeclToImportDecl.get(deferBlockDep.declaration.node) ?? null; + const node = deferBlockDep.declaration.node; + const importDecl = resolution.deferrableDeclToImportDecl.get(node) ?? null; if (importDecl !== null && this.deferredSymbolTracker.canDefer(importDecl)) { deferBlockDep.isDeferrable = true; deferBlockDep.importPath = (importDecl.moduleSpecifier as ts.StringLiteral).text; deferBlockDep.isDefaultImport = isDefaultImport(importDecl); - deferrableTypes.push(deferBlockDep as R3DeferPerComponentDependency); + + // The same dependency may be used across multiple deferred blocks. De-duplicate it + // because it can throw off other logic further down the compilation pipeline. + // Note that the logic above needs to run even if the dependency is seen before, + // because the object literals are different between each block. + if (!seenDeps.has(node)) { + seenDeps.add(node); + deferrableTypes.push(deferBlockDep as R3DeferPerComponentDependency); + } } } } + return deferrableTypes; } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/GOLDEN_PARTIAL.js index a53d28b5523..b1aa2bed948 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/GOLDEN_PARTIAL.js @@ -1122,3 +1122,99 @@ i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDE ****************************************************************************************************/ export {}; +/**************************************************************************************************** + * PARTIAL FILE: deferred_with_duplicate_external_dep_lazy.js + ****************************************************************************************************/ +import { Directive } from '@angular/core'; +import * as i0 from "@angular/core"; +export class DuplicateLazyDep { +} +DuplicateLazyDep.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: DuplicateLazyDep, deps: [], target: i0.ɵɵFactoryTarget.Directive }); +DuplicateLazyDep.ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: DuplicateLazyDep, isStandalone: true, selector: "duplicate-lazy-dep", ngImport: i0 }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: DuplicateLazyDep, decorators: [{ + type: Directive, + args: [{ selector: 'duplicate-lazy-dep' }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: deferred_with_duplicate_external_dep_lazy.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class DuplicateLazyDep { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵdir: i0.ɵɵDirectiveDeclaration; +} + +/**************************************************************************************************** + * PARTIAL FILE: deferred_with_duplicate_external_dep_other.js + ****************************************************************************************************/ +import { Directive } from '@angular/core'; +import * as i0 from "@angular/core"; +export class OtherLazyDep { +} +OtherLazyDep.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: OtherLazyDep, deps: [], target: i0.ɵɵFactoryTarget.Directive }); +OtherLazyDep.ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "14.0.0", version: "0.0.0-PLACEHOLDER", type: OtherLazyDep, isStandalone: true, selector: "other-lazy-dep", ngImport: i0 }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: OtherLazyDep, decorators: [{ + type: Directive, + args: [{ selector: 'other-lazy-dep' }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: deferred_with_duplicate_external_dep_other.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class OtherLazyDep { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵdir: i0.ɵɵDirectiveDeclaration; +} + +/**************************************************************************************************** + * PARTIAL FILE: deferred_with_duplicate_external_dep.js + ****************************************************************************************************/ +import { Component } from '@angular/core'; +import * as i0 from "@angular/core"; +export class MyApp { +} +MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component }); +MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, isStandalone: true, selector: "ng-component", ngImport: i0, template: ` + @defer { + + } + + @defer { + + } + + @defer { + + } + `, isInline: true, deferBlockDependencies: [() => [import("./deferred_with_duplicate_external_dep_lazy").then(m => m.DuplicateLazyDep)], () => [import("./deferred_with_duplicate_external_dep_lazy").then(m => m.DuplicateLazyDep)], () => [import("./deferred_with_duplicate_external_dep_other").then(m => m.OtherLazyDep)]] }); +i0.ɵɵngDeclareClassMetadataAsync({ minVersion: "18.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, resolveDeferredDeps: () => [import("./deferred_with_duplicate_external_dep_lazy").then(m => m.DuplicateLazyDep), import("./deferred_with_duplicate_external_dep_other").then(m => m.OtherLazyDep)], resolveMetadata: (DuplicateLazyDep, OtherLazyDep) => ({ decorators: [{ + type: Component, + args: [{ + template: ` + @defer { + + } + + @defer { + + } + + @defer { + + } + `, + imports: [DuplicateLazyDep, OtherLazyDep], + }] + }], ctorParameters: null, propDecorators: null }) }); + +/**************************************************************************************************** + * PARTIAL FILE: deferred_with_duplicate_external_dep.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class MyApp { + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/TEST_CASES.json index a027e6c5853..93cbe6d13d3 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/TEST_CASES.json @@ -290,6 +290,25 @@ "failureMessage": "Incorrect template" } ] + }, + { + "description": "should handle a component with deferred blocks that share the same dependency", + "inputFiles": [ + "deferred_with_duplicate_external_dep.ts", + "deferred_with_duplicate_external_dep_lazy.ts", + "deferred_with_duplicate_external_dep_other.ts" + ], + "expectations": [ + { + "files": [ + { + "expected": "deferred_with_duplicate_external_dep_template.js", + "generated": "deferred_with_duplicate_external_dep.js" + } + ], + "failureMessage": "Incorrect template" + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep.ts new file mode 100644 index 00000000000..59cbe0f4bf3 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep.ts @@ -0,0 +1,21 @@ +import {Component} from '@angular/core'; +import {DuplicateLazyDep} from './deferred_with_duplicate_external_dep_lazy'; +import {OtherLazyDep} from './deferred_with_duplicate_external_dep_other'; + +@Component({ + template: ` + @defer { + + } + + @defer { + + } + + @defer { + + } + `, + imports: [DuplicateLazyDep, OtherLazyDep], +}) +export class MyApp {} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_lazy.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_lazy.ts new file mode 100644 index 00000000000..8321df6e6a6 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_lazy.ts @@ -0,0 +1,4 @@ +import {Directive} from '@angular/core'; + +@Directive({selector: 'duplicate-lazy-dep'}) +export class DuplicateLazyDep {} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_other.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_other.ts new file mode 100644 index 00000000000..4414f06e6d3 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_other.ts @@ -0,0 +1,4 @@ +import {Directive} from '@angular/core'; + +@Directive({selector: 'other-lazy-dep'}) +export class OtherLazyDep {} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_template.js new file mode 100644 index 00000000000..ef430e56696 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_deferred/deferred_with_duplicate_external_dep_template.js @@ -0,0 +1,43 @@ +const MyApp_Defer_1_DepsFn = () => [import("./deferred_with_duplicate_external_dep_lazy").then(m => m.DuplicateLazyDep)]; +// NOTE: in linked tests there is one more loader here, because linked compilation doesn't have the ability to de-dupe identical functions. +… +const MyApp_Defer_7_DepsFn = () => [import("./deferred_with_duplicate_external_dep_other").then(m => m.OtherLazyDep)]; + +… + +$r3$.ɵɵdefineComponent({ + … + template: function MyApp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵtemplate(0, MyApp_Defer_0_Template, 1, 0); + $r3$.ɵɵdefer(1, 0, MyApp_Defer_1_DepsFn); + $r3$.ɵɵdeferOnIdle(); + $r3$.ɵɵtemplate(3, MyApp_Defer_3_Template, 1, 0); + // NOTE: does not check the function name, because linked compilation doesn't have the ability to de-dupe identical functions. + $r3$.ɵɵdefer(4, 3, …); + $r3$.ɵɵdeferOnIdle(); + $r3$.ɵɵtemplate(6, MyApp_Defer_6_Template, 1, 0); + $r3$.ɵɵdefer(7, 6, MyApp_Defer_7_DepsFn); + $r3$.ɵɵdeferOnIdle(); + } + }, + encapsulation: 2 +}); + +… + +(() => { + (typeof ngDevMode === "undefined" || ngDevMode) && $r3$.ɵsetClassMetadataAsync(MyApp, () => [ + import("./deferred_with_duplicate_external_dep_lazy").then(m => m.DuplicateLazyDep), + import("./deferred_with_duplicate_external_dep_other").then(m => m.OtherLazyDep) + ], (DuplicateLazyDep, OtherLazyDep) => { + $r3$.ɵsetClassMetadata(MyApp, [{ + type: Component, + args: [{ + template: …, + // NOTE: there's a ... after the `imports`, because linked compilation produces a trailing comma while full compilation doesn't. + imports: [DuplicateLazyDep, OtherLazyDep]… + }] + }], null, null); + }); +})();