From cb833f988367fb95ae9e707828153581e6534dad Mon Sep 17 00:00:00 2001 From: Payam Valadkhan Date: Mon, 18 Sep 2023 18:06:07 -0400 Subject: [PATCH 1/3] fix(compiler-cli): fix NgModule injector def in local compilation mode when imports/exports are non-array expressions Current implementation assumes that NgModule imports/exports fields are always arrays and thus it concats them for the injector definition. But this is not always the case and imports/exports could be non-arrays such as const variable. Such pattern happens in g3 and so must be addressed. --- .../annotations/ng_module/src/handler.ts | 20 +++-- .../test/ngtsc/local_compilation_spec.ts | 88 ++++++++++++++++++- 2 files changed, 100 insertions(+), 8 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts index b3a5313f61cf7..a5d33eb5c1045 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts @@ -445,12 +445,22 @@ export class NgModuleDecoratorHandler implements }; if (this.compilationMode === CompilationMode.LOCAL) { - if (rawImports !== null && ts.isArrayLiteralExpression(rawImports) && rawImports.elements) { - injectorMetadata.imports.push(...rawImports.elements.map(n => new WrappedNodeExpr(n))); - } + // Adding NgModule's raw imports/exports to the injector's imports field in local compilation + // mode. + for (const exp of [rawImports, rawExports]) { + if (exp === null) { + continue; + } - if (rawExports !== null && ts.isArrayLiteralExpression(rawExports) && rawExports.elements) { - injectorMetadata.imports.push(...rawExports.elements.map(n => new WrappedNodeExpr(n))); + if (ts.isArrayLiteralExpression(exp)) { + // If array expression then add it entry-by-entry to the injector imports + if (exp.elements) { + injectorMetadata.imports.push(...exp.elements.map(n => new WrappedNodeExpr(n))); + } + } else { + // if not array expression then add it as is to the injector's imports field. + injectorMetadata.imports.push(new WrappedNodeExpr(exp)); + } } } diff --git a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts index 2a8084e797c06..35325c9606de3 100644 --- a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts +++ b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts @@ -62,7 +62,7 @@ runInEachFileSystem(() => { expect(jsContents).toContain('MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({})'); }); - it('should include raw module imports (including forward refs) in the injector def imports', + it('should include raw module imports array elements (including forward refs) in the injector def imports', () => { env.write('test.ts', ` import {NgModule, forwardRef} from '@angular/core'; @@ -90,7 +90,36 @@ runInEachFileSystem(() => { 'MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({ imports: [SubModule1, forwardRef(() => SubModule2), LocalModule1, forwardRef(() => LocalModule2)] })'); }); - it('should include raw module exports (including forward refs) in the injector def imports', + it('should include non-array raw module imports as it is in the injector def imports', () => { + env.write('test.ts', ` + import {NgModule, forwardRef} from '@angular/core'; + import {SubModule1} from './some-where'; + import {SubModule2} from './another-where'; + + const NG_IMPORTS = [SubModule1, forwardRef(() => SubModule2), LocalModule1, forwardRef(() => LocalModule2)]; + + @NgModule({}) + class LocalModule1 {} + + @NgModule({ + imports: NG_IMPORTS, + }) + export class MainModule { + } + + @NgModule({}) + class LocalModule2 {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + + expect(jsContents) + .toContain( + 'MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({ imports: [NG_IMPORTS] })'); + }); + + it('should include raw module exports array elements (including forward refs) in the injector def imports', () => { env.write('test.ts', ` import {NgModule, forwardRef} from '@angular/core'; @@ -118,7 +147,37 @@ runInEachFileSystem(() => { 'MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({ imports: [SubModule1, forwardRef(() => SubModule2), LocalModule1, forwardRef(() => LocalModule2)] })'); }); - it('should combine raw module imports and exports (including forward refs) in the injector def imports', + it('should include non-array raw module exports (including forward refs) in the injector def imports', + () => { + env.write('test.ts', ` + import {NgModule, forwardRef} from '@angular/core'; + import {SubModule1} from './some-where'; + import {SubModule2} from './another-where'; + + const NG_EXPORTS = [SubModule1, forwardRef(() => SubModule2), LocalModule1, forwardRef(() => LocalModule2)]; + + @NgModule({}) + class LocalModule1 {} + + @NgModule({ + exports: NG_EXPORTS, + }) + export class MainModule { + } + + @NgModule({}) + class LocalModule2 {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + + expect(jsContents) + .toContain( + 'MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({ imports: [NG_EXPORTS] })'); + }); + + it('should concat raw module imports and exports arrays (including forward refs) in the injector def imports', () => { env.write('test.ts', ` import {NgModule, forwardRef} from '@angular/core'; @@ -140,6 +199,29 @@ runInEachFileSystem(() => { .toContain( 'MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({ imports: [SubModule1, forwardRef(() => SubModule2), SubModule3, forwardRef(() => SubModule4)] })'); }); + + it('should combines non-array raw module imports and exports (including forward refs) in the injector def imports', + () => { + env.write('test.ts', ` + import {NgModule, forwardRef} from '@angular/core'; + import {NG_IMPORTS} from './some-where'; + import {NG_EXPORTS} from './another-where'; + + @NgModule({ + imports: NG_IMPORTS, + exports: NG_EXPORTS, + }) + export class MainModule { + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + + expect(jsContents) + .toContain( + 'MainModule.ɵinj = /*@__PURE__*/ i0.ɵɵdefineInjector({ imports: [NG_IMPORTS, NG_EXPORTS] })'); + }); }); describe('component dependencies', () => { From 007408ce601ce7dc82a1b22e61d1d245fb72018d Mon Sep 17 00:00:00 2001 From: Payam Valadkhan Date: Mon, 18 Sep 2023 18:50:39 -0400 Subject: [PATCH 2/3] fix(compiler-cli): allow non-array imports for standalone component in local compilation mode Currently the compiler in local mode assumes that the standalone component imports are array expressions. This is not always true as they can be const variables as well. This change allow non-array expressions for standalone component imports field and passes that expression to the downstream tools such as deps tracker to compute the component's deps in runtime. --- .../annotations/component/src/handler.ts | 5 +--- .../test/ngtsc/local_compilation_spec.ts | 30 +++++++++++++++++-- 2 files changed, 29 insertions(+), 6 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 cbc3a053b327d..0c9dfa5fe079a 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -468,10 +468,7 @@ export class ComponentDecoratorHandler implements viewProviders: wrappedViewProviders, i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath, - rawImports: (rawImports !== null && ts.isArrayLiteralExpression(rawImports) && - rawImports.elements.length > 0) ? - new WrappedNodeExpr(rawImports) : - undefined, + rawImports: rawImports !== null ? new WrappedNodeExpr(rawImports) : undefined, }, typeCheckMeta: extractDirectiveTypeCheckMeta(node, inputs, this.reflector), classMetadata: this.includeClassMetadata ? diff --git a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts index 35325c9606de3..ae864dca939e1 100644 --- a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts +++ b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts @@ -276,7 +276,33 @@ runInEachFileSystem(() => { 'dependencies: i0.ɵɵgetComponentDepsFactory(MainComponent, [SomeThing, forwardRef(() => SomeThing2)])'); }); - it('should not generate ɵɵgetComponentDepsFactory for standalone component with empty imports', + it('should generate ɵɵgetComponentDepsFactory with raw non-array imports as second param for component def dependencies - for standalone component with non-empty imports', + () => { + env.write('test.ts', ` + import {Component, forwardRef} from '@angular/core'; + import {SomeThing} from 'some-where'; + import {SomeThing2} from 'some-where2'; + + const NG_IMPORTS = [SomeThing, forwardRef(()=>SomeThing2)]; + + @Component({ + standalone: true, + imports: NG_IMPORTS, + selector: 'test-main', + template: 'Hello world!', + }) + export class MainComponent { + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + + expect(jsContents) + .toContain('dependencies: i0.ɵɵgetComponentDepsFactory(MainComponent, NG_IMPORTS)'); + }); + + it('should generate ɵɵgetComponentDepsFactory with empty array as secon d arg for standalone component with empty imports', () => { env.write('test.ts', ` import {Component} from '@angular/core'; @@ -295,7 +321,7 @@ runInEachFileSystem(() => { const jsContents = env.getContents('test.js'); expect(jsContents) - .toContain('dependencies: i0.ɵɵgetComponentDepsFactory(MainComponent)'); + .toContain('dependencies: i0.ɵɵgetComponentDepsFactory(MainComponent, [])'); }); it('should not generate ɵɵgetComponentDepsFactory for standalone component with no imports', From 6431238311580d28ddbe33ec222d745ba38ad4a6 Mon Sep 17 00:00:00 2001 From: Payam Valadkhan Date: Mon, 18 Sep 2023 19:11:45 -0400 Subject: [PATCH 3/3] refactor(core): include injector info for standalone components in local compilation mode Standalone component need to include the imported NgModules as part of their dependencies in order to be able to use the injection tokens coming from these NgModules. To do so, in this change the imported NgModules are included in the standalone component compilation scope. --- packages/core/src/render3/deps_tracker/api.ts | 12 ++++++++- .../src/render3/deps_tracker/deps_tracker.ts | 3 +++ .../core/test/render3/deps_tracker_spec.ts | 27 ++++++++++++++----- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/packages/core/src/render3/deps_tracker/api.ts b/packages/core/src/render3/deps_tracker/api.ts index 1d73e14044f61..9f78df2cc0387 100644 --- a/packages/core/src/render3/deps_tracker/api.ts +++ b/packages/core/src/render3/deps_tracker/api.ts @@ -29,6 +29,16 @@ interface ScopeData { isPoisoned?: boolean; } +/** + * Represents scope data for standalone components as calculated during runtime by the deps + * tracker. + */ +interface StandaloneCompScopeData extends ScopeData { + // Standalone components include the imported NgModules in their dependencies in order to + // determine their injector info. The following field stores the set of such NgModules. + ngModules: Set>; +} + /** Represents scope data for NgModule as calculated during runtime by the deps tracker. */ export interface NgModuleScope { compilation: ScopeData; @@ -39,7 +49,7 @@ export interface NgModuleScope { * Represents scope data for standalone component as calculated during runtime by the deps tracker. */ export interface StandaloneComponentScope { - compilation: ScopeData; + compilation: StandaloneCompScopeData; } /** Component dependencies info as calculated during runtime by the deps tracker. */ diff --git a/packages/core/src/render3/deps_tracker/deps_tracker.ts b/packages/core/src/render3/deps_tracker/deps_tracker.ts index ddcba93391e04..2e914166c6d92 100644 --- a/packages/core/src/render3/deps_tracker/deps_tracker.ts +++ b/packages/core/src/render3/deps_tracker/deps_tracker.ts @@ -82,6 +82,7 @@ class DepsTracker implements DepsTrackerApi { dependencies: [ ...scope.compilation.directives, ...scope.compilation.pipes, + ...scope.compilation.ngModules, ] }; } else { @@ -246,6 +247,7 @@ class DepsTracker implements DepsTrackerApi { // Standalone components are always able to self-reference. directives: new Set([type]), pipes: new Set(), + ngModules: new Set(), }, }; @@ -261,6 +263,7 @@ class DepsTracker implements DepsTrackerApi { } if (isNgModule(imported)) { + ans.compilation.ngModules.add(imported); const importedScope = this.getNgModuleScope(imported); // Short-circuit if an imported NgModule has corrupted exported scope. diff --git a/packages/core/test/render3/deps_tracker_spec.ts b/packages/core/test/render3/deps_tracker_spec.ts index 8ae8f7bd990f1..af639ec2fc8ee 100644 --- a/packages/core/test/render3/deps_tracker_spec.ts +++ b/packages/core/test/render3/deps_tracker_spec.ts @@ -701,6 +701,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([]), directives: new Set([MainComponent]), + ngModules: new Set([]), }); }); @@ -726,6 +727,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([]), }); }); @@ -751,6 +753,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([]), }); }); @@ -767,7 +770,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation.isPoisoned).toBeTrue(); }); - it('should include the exported scope of an imported module in the compilation scope', () => { + it('should include the imported module and its exported scope in the compilation scope', () => { @Directive({}) class Directive1 { } @@ -799,10 +802,11 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([SubSubModule]), }); }); - it('should include the exported scope of an imported module in the compilation scope - case of nested array imports', + it('should include the imported module and its exported scope in the compilation scope - case of nested array imports', () => { @Directive({}) class Directive1 { @@ -835,6 +839,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([SubSubModule]), }); }); @@ -878,6 +883,7 @@ describe('runtime dependency tracker', () => { pipes: new Set([Pipe1, SubModulePipe]), directives: new Set( [MainComponent, Component1, Directive1, SubModuleComponent, SubModuleDirective]), + ngModules: new Set([SubModule]), }); }); @@ -922,6 +928,7 @@ describe('runtime dependency tracker', () => { pipes: new Set([Pipe1, SubModulePipe]), directives: new Set( [MainComponent, Component1, Directive1, SubModuleComponent, SubModuleDirective]), + ngModules: new Set([SubModule]), }); }); @@ -946,6 +953,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([]), }); ans = depsTracker.getStandaloneComponentScope(MainComponent as ComponentType, []); @@ -953,6 +961,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([]), }); }); @@ -979,6 +988,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([Pipe1]), directives: new Set([MainComponent, Component1, Directive1]), + ngModules: new Set([]), }); depsTracker.clearScopeCacheFor(MainComponent as ComponentType); @@ -987,6 +997,7 @@ describe('runtime dependency tracker', () => { expect(ans.compilation).toEqual({ pipes: new Set([]), directives: new Set([MainComponent]), + ngModules: new Set([]), }); }); }); @@ -1172,7 +1183,7 @@ describe('runtime dependency tracker', () => { expect(ans.dependencies).toEqual([]); }); - it('should include the exported scope of imported module', () => { + it('should include the imported module and its exported scope', () => { @Component({}) class Component1 { } @@ -1199,11 +1210,15 @@ describe('runtime dependency tracker', () => { depsTracker.getComponentDependencies(MainComponent as ComponentType, [SubModule]); expect(ans.dependencies).toEqual(jasmine.arrayWithExactContents([ - MainComponent, Component1, Directive1, Pipe1 + MainComponent, + Component1, + Directive1, + Pipe1, + SubModule, ])); }); - it('should include the exported scope of imported forward ref module', () => { + it('should include the imported forward ref module and its exported scope', () => { @Component({}) class Component1 { } @@ -1230,7 +1245,7 @@ describe('runtime dependency tracker', () => { MainComponent as ComponentType, [forwardRef(() => SubModule)]); expect(ans.dependencies).toEqual(jasmine.arrayWithExactContents([ - MainComponent, Component1, Directive1, Pipe1 + MainComponent, Component1, Directive1, Pipe1, SubModule ])); });