From a006edefd1ff65d8115e6e6d001a4c9a18c2888b Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Wed, 1 Jun 2022 15:40:56 -0700 Subject: [PATCH] refactor(compiler-cli): introduce `onlyPublishPublicTypingsForNgModules` (#45894) When generating .d.ts metadata for NgModules, by default we emit type references to their declarations, imports, and exports. However, this information is not necessarily useful to consumers. References to private directives (those that aren't exported by the NgModule) for example aren't at all useful as they can only affect other components declared in the NgModule. References to imports are of limited usefulness - they might be helpful for an IDE to understand the DI structure of an application, but aren't at all used by a downstream compiler. Generating this metadata is not without cost. When an incremental build system uses changes in inputs to determine when a rebuild is necessary, any changes in .d.ts files might cause downstream targets to rebuild. If those .d.ts changes are in the "private" side of the NgModule (imports or non- exported directives/pipes), then these rebuilds are wholly unnecessary. This commit introduces the `onlyPublishPublicTypingsForNgModules` flag for the compiler. When this flag is set, the compiler will filter the emitted references in NgModule .d.ts output and only reference those directives/ pipes that are exported from the NgModule (its public API surface). Omitting the flag preserves the existing behavior of emitting all references, both public and private. This is especially useful for build systems such as Bazel. PR Close #45894 --- .../compiler-cli/compiler_options.md | 1 + .../partial_ng_module_linker_1.ts | 2 + .../ngcc/src/analysis/decoration_analyzer.ts | 2 +- .../annotations/ng_module/src/handler.ts | 24 +++++-- .../ng_module/test/ng_module_spec.ts | 3 +- .../src/ngtsc/core/api/src/public_options.ts | 11 +++ .../src/ngtsc/core/src/compiler.ts | 3 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 70 +++++++++++++++++-- packages/compiler/src/jit_compiler_facade.ts | 2 + .../src/render3/r3_module_compiler.ts | 26 ++++++- 10 files changed, 130 insertions(+), 14 deletions(-) diff --git a/goldens/public-api/compiler-cli/compiler_options.md b/goldens/public-api/compiler-cli/compiler_options.md index 89f773eff89c2..9433d6f9efc7e 100644 --- a/goldens/public-api/compiler-cli/compiler_options.md +++ b/goldens/public-api/compiler-cli/compiler_options.md @@ -8,6 +8,7 @@ export interface BazelAndG3Options { annotateForClosureCompiler?: boolean; generateDeepReexports?: boolean; + onlyPublishPublicTypingsForNgModules?: boolean; } // @public diff --git a/packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_ng_module_linker_1.ts b/packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_ng_module_linker_1.ts index d34c35c160e53..e62f549ed78fd 100644 --- a/packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_ng_module_linker_1.ts +++ b/packages/compiler-cli/linker/src/file_linker/partial_linkers/partial_ng_module_linker_1.ts @@ -45,6 +45,8 @@ export function toR3NgModuleMeta( adjacentType: wrappedType, bootstrap: [], declarations: [], + publicDeclarationTypes: null, + includeImportTypes: true, imports: [], exports: [], selectorScopeMode: supportJit ? R3SelectorScopeMode.Inline : R3SelectorScopeMode.Omit, diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index 160da5412acbf..f23dd3ff23ab6 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -141,7 +141,7 @@ export class DecorationAnalyzer { this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry, this.scopeRegistry, this.referencesRegistry, this.isCore, this.refEmitter, /* factoryTracker */ null, !!this.compilerOptions.annotateForClosureCompiler, - this.injectableRegistry, NOOP_PERF_RECORDER), + /* onlyPublishPublicTypings */ false, this.injectableRegistry, NOOP_PERF_RECORDER), ]; compiler = new NgccTraitCompiler(this.handlers, this.reflectionHost); migrations: Migration[] = [ 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 75d56c9008479..b3df9db2aeab4 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 @@ -131,7 +131,7 @@ export class NgModuleDecoratorHandler implements private scopeRegistry: LocalModuleScopeRegistry, private referencesRegistry: ReferencesRegistry, private isCore: boolean, private refEmitter: ReferenceEmitter, private factoryTracker: FactoryTracker|null, - private annotateForClosureCompiler: boolean, + private annotateForClosureCompiler: boolean, private onlyPublishPublicTypings: boolean, private injectableRegistry: InjectableClassRegistry, private perf: PerfRecorder) {} readonly precedence = HandlerPrecedence.PRIMARY; @@ -274,13 +274,24 @@ export class NgModuleDecoratorHandler implements typeContext = typeNode.getSourceFile(); } + + const exportedNodes = new Set(exportRefs.map(ref => ref.node)); + const declarations: R3Reference[] = []; + const exportedDeclarations: Expression[] = []; + const bootstrap = bootstrapRefs.map( bootstrap => this._toR3Reference( bootstrap.getOriginForDiagnostics(meta, node.name), bootstrap, valueContext, typeContext)); - const declarations = declarationRefs.map( - decl => this._toR3Reference( - decl.getOriginForDiagnostics(meta, node.name), decl, valueContext, typeContext)); + + for (const ref of declarationRefs) { + const decl = this._toR3Reference( + ref.getOriginForDiagnostics(meta, node.name), ref, valueContext, typeContext); + declarations.push(decl); + if (exportedNodes.has(ref.node)) { + exportedDeclarations.push(decl.type); + } + } const imports = importRefs.map( imp => this._toR3Reference( imp.getOriginForDiagnostics(meta, node.name), imp, valueContext, typeContext)); @@ -288,6 +299,7 @@ export class NgModuleDecoratorHandler implements exp => this._toR3Reference( exp.getOriginForDiagnostics(meta, node.name), exp, valueContext, typeContext)); + const isForwardReference = (ref: R3Reference) => isExpressionForwardReference(ref.value, node.name!, valueContext); const containsForwardDecls = bootstrap.some(isForwardReference) || @@ -304,8 +316,12 @@ export class NgModuleDecoratorHandler implements adjacentType, bootstrap, declarations, + publicDeclarationTypes: this.onlyPublishPublicTypings ? exportedDeclarations : null, exports, imports, + // Imported types are generally private, so include them unless restricting the .d.ts emit to + // only public types. + includeImportTypes: !this.onlyPublishPublicTypings, containsForwardDecls, id, // Use `ɵɵsetNgModuleScope` to patch selector scopes onto the generated definition in a diff --git a/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts index f884bd47cd621..0af3c2f3fd1ef 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/ng_module/test/ng_module_spec.ts @@ -72,7 +72,8 @@ runInEachFileSystem(() => { const handler = new NgModuleDecoratorHandler( reflectionHost, evaluator, metaReader, metaRegistry, scopeRegistry, referencesRegistry, /* isCore */ false, refEmitter, /* factoryTracker */ null, - /* annotateForClosureCompiler */ false, injectableRegistry, NOOP_PERF_RECORDER); + /* annotateForClosureCompiler */ false, /* onlyPublishPublicTypings */ false, + injectableRegistry, NOOP_PERF_RECORDER); const TestModule = getDeclaration(program, _('/entry.ts'), 'TestModule', isNamedClassDeclaration); const detected = diff --git a/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts b/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts index 2075990831cf9..e515568255273 100644 --- a/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts +++ b/packages/compiler-cli/src/ngtsc/core/api/src/public_options.ts @@ -338,6 +338,17 @@ export interface BazelAndG3Options { */ generateDeepReexports?: boolean; + /** + * The `.d.ts` file for NgModules contain type pointers to their declarations, imports, and + * exports. Without this flag, the generated type definition will include + * components/directives/pipes/NgModules that are declared or imported locally in the NgModule and + * not necessarily exported to consumers. + * + * With this flag set, the type definition generated in the `.d.ts` for an NgModule will be + * filtered to only list those types which are publicly exported by the NgModule. + */ + onlyPublishPublicTypingsForNgModules?: boolean; + /** * Insert JSDoc type annotations needed by Closure Compiler */ diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index fce0d651f02f2..43996ccb9bb15 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -1044,7 +1044,8 @@ export class NgCompiler { new NgModuleDecoratorHandler( reflector, evaluator, metaReader, metaRegistry, ngModuleScopeRegistry, referencesRegistry, isCore, refEmitter, this.adapter.factoryTracker, this.closureCompilerEnabled, - injectableRegistry, this.delegatingPerfRecorder), + this.options.onlyPublishPublicTypingsForNgModules ?? false, injectableRegistry, + this.delegatingPerfRecorder), ]; const traitCompiler = new TraitCompiler( diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 1f42439a26677..67d8429efda70 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -476,6 +476,67 @@ function allTests(os: string) { } }); + it('should only list public declarations in NgModule .d.ts when requested', () => { + env.tsconfig({ + onlyPublishPublicTypingsForNgModules: true, + }); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({selector: 'internal'}) + export class InternalDir {} + + @Directive({selector: 'external'}) + export class ExternalDir {} + + @NgModule({ + declarations: [InternalDir, ExternalDir], + exports: [ExternalDir], + }) + export class Module {} + `); + + env.driveMain(); + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents) + .toContain( + 'static ɵmod: i0.ɵɵNgModuleDeclaration'); + }); + + it('should not list imports in NgModule .d.ts when requested', () => { + env.tsconfig({ + onlyPublishPublicTypingsForNgModules: true, + }); + env.write('test.ts', ` + import {Directive, NgModule} from '@angular/core'; + + @Directive({selector: 'internal'}) + export class InternalDir {} + + @NgModule({ + declarations: [InternalDir], + exports: [InternalDir], + }) + export class DepModule {} + + @Directive({selector: 'external'}) + export class ExternalDir {} + + @NgModule({ + imports: [DepModule], + }) + export class Module {} + `); + + env.driveMain(); + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents) + .toContain( + 'static ɵmod: i0.ɵɵNgModuleDeclaration'); + }); + // This test triggers the Tsickle compiler which asserts that the file-paths // are valid for the real OS. When on non-Windows systems it doesn't like paths // that start with `C:`. @@ -7328,10 +7389,11 @@ function allTests(os: string) { expect(diags.length).toBe(0); }); - // TODO(alxhub): this test never worked correctly, as it used to declare a constructor with a - // body, which real declaration files don't have. Without the body, the ReflectionHost used to - // not return any constructor data, preventing an error from showing. That bug was fixed, but - // the error for declaration files is disabled until g3 can be updated. + // TODO(alxhub): this test never worked correctly, as it used to declare a constructor + // with a body, which real declaration files don't have. Without the body, the + // ReflectionHost used to not return any constructor data, preventing an error from + // showing. That bug was fixed, but the error for declaration files is disabled until g3 + // can be updated. xit('should error when an undecorated class with a non-trivial constructor in a declaration file is provided via useClass', () => { env.write('node_modules/@angular/core/testing/index.d.ts', ` diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts index 76d2554fd03f9..9170aee811526 100644 --- a/packages/compiler/src/jit_compiler_facade.ts +++ b/packages/compiler/src/jit_compiler_facade.ts @@ -131,7 +131,9 @@ export class CompilerFacadeImpl implements CompilerFacade { adjacentType: new WrappedNodeExpr(facade.type), bootstrap: facade.bootstrap.map(wrapReference), declarations: facade.declarations.map(wrapReference), + publicDeclarationTypes: null, // only needed for types in AOT imports: facade.imports.map(wrapReference), + includeImportTypes: true, exports: facade.exports.map(wrapReference), selectorScopeMode: R3SelectorScopeMode.Inline, containsForwardDecls: false, diff --git a/packages/compiler/src/render3/r3_module_compiler.ts b/packages/compiler/src/render3/r3_module_compiler.ts index bb4e8ea23cc29..2023c0b813271 100644 --- a/packages/compiler/src/render3/r3_module_compiler.ts +++ b/packages/compiler/src/render3/r3_module_compiler.ts @@ -81,11 +81,22 @@ export interface R3NgModuleMetadata { */ declarations: R3Reference[]; + /** + * Those declarations which should be visible to downstream consumers. If not specified, all + * declarations are made visible to downstream consumers. + */ + publicDeclarationTypes: o.Expression[]|null; + /** * An array of expressions representing the imports of the module. */ imports: R3Reference[]; + /** + * Whether or not to include `imports` in generated type declarations. + */ + includeImportTypes: boolean; + /** * An array of expressions representing the exports of the module. */ @@ -248,10 +259,14 @@ export function compileNgModuleDeclarationExpression(meta: R3DeclareNgModuleFaca } export function createNgModuleType( - {type: moduleType, declarations, imports, exports}: R3NgModuleMetadata): o.ExpressionType { + {type: moduleType, declarations, exports, imports, includeImportTypes, publicDeclarationTypes}: + R3NgModuleMetadata): o.ExpressionType { return new o.ExpressionType(o.importExpr(R3.NgModuleDeclaration, [ - new o.ExpressionType(moduleType.type), tupleTypeOf(declarations), tupleTypeOf(imports), - tupleTypeOf(exports) + new o.ExpressionType(moduleType.type), + publicDeclarationTypes === null ? tupleTypeOf(declarations) : + tupleOfTypes(publicDeclarationTypes), + includeImportTypes ? tupleTypeOf(imports) : o.NONE_TYPE, + tupleTypeOf(exports), ])); } @@ -308,3 +323,8 @@ function tupleTypeOf(exp: R3Reference[]): o.Type { const types = exp.map(ref => o.typeofExpr(ref.type)); return exp.length > 0 ? o.expressionType(o.literalArr(types)) : o.NONE_TYPE; } + +function tupleOfTypes(types: o.Expression[]): o.Type { + const typeofTypes = types.map(type => o.typeofExpr(type)); + return types.length > 0 ? o.expressionType(o.literalArr(typeofTypes)) : o.NONE_TYPE; +}