-
Notifications
You must be signed in to change notification settings - Fork 27.2k
ngcc: re-export private NgModule declarations #26906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e842d18
9766024
fc6a58e
c918dc3
1eaa2d8
171dad2
289684f
fc12c0a
8111d4b
9b5366e
51f490d
40315aa
c1139c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /** | ||
| * @license | ||
| * Copyright Google Inc. All Rights Reserved. | ||
| * | ||
| * Use of this source code is governed by an MIT-style license that can be | ||
| * found in the LICENSE file at https://angular.io/license | ||
| */ | ||
|
|
||
| import * as ts from 'typescript'; | ||
| import {ReferencesRegistry} from '../../../ngtsc/annotations'; | ||
| import {Declaration, ReflectionHost} from '../../../ngtsc/host'; | ||
| import {Reference, ResolvedReference} from '../../../ngtsc/metadata'; | ||
| import {hasNameIdentifier} from '../utils'; | ||
|
|
||
| /** | ||
| * This is a place for DecoratorHandlers to register references that they | ||
| * find in their analysis of the code. | ||
| * | ||
| * This registry is used to ensure that these references are publicly exported | ||
| * from libraries that are compiled by ngcc. | ||
| */ | ||
| export class NgccReferencesRegistry implements ReferencesRegistry { | ||
| private map = new Map<ts.Identifier, Declaration>(); | ||
|
|
||
| constructor(private host: ReflectionHost) {} | ||
|
|
||
| /** | ||
| * Register one or more references in the registry. | ||
| * Only `ResolveReference` references are stored. Other types are ignored. | ||
| * @param references A collection of references to register. | ||
| */ | ||
| add(...references: Reference<ts.Declaration>[]): void { | ||
| references.forEach(ref => { | ||
| // Only store resolved references. We are not interested in literals. | ||
| if (ref instanceof ResolvedReference && hasNameIdentifier(ref.node)) { | ||
| const declaration = this.host.getDeclarationOfIdentifier(ref.node.name); | ||
| if (declaration && hasNameIdentifier(declaration.node)) { | ||
| this.map.set(declaration.node.name, declaration); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Create and return a mapping for the registered resolved references. | ||
| * @returns A map of reference identifiers to reference declarations. | ||
| */ | ||
| getDeclarationMap(): Map<ts.Identifier, Declaration> { return this.map; } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| /** | ||
| * @license | ||
| * Copyright Google Inc. All Rights Reserved. | ||
| * | ||
| * Use of this source code is governed by an MIT-style license that can be | ||
| * found in the LICENSE file at https://angular.io/license | ||
| */ | ||
| import * as ts from 'typescript'; | ||
|
|
||
| import {ReferencesRegistry} from '../../../ngtsc/annotations'; | ||
| import {Declaration} from '../../../ngtsc/host'; | ||
| import {NgccReflectionHost} from '../host/ngcc_host'; | ||
| import {hasNameIdentifier, isDefined} from '../utils'; | ||
|
|
||
| export interface ExportInfo { | ||
| identifier: string; | ||
| from: string; | ||
| dtsFrom: string|null; | ||
| } | ||
| export type PrivateDeclarationsAnalyses = ExportInfo[]; | ||
|
|
||
| /** | ||
| * This class will analyze a program to find all the declared classes | ||
| * (i.e. on an NgModule) that are not publicly exported via an entry-point. | ||
| */ | ||
| export class PrivateDeclarationsAnalyzer { | ||
| constructor(private host: NgccReflectionHost, private referencesRegistry: ReferencesRegistry) {} | ||
|
|
||
| analyzeProgram(program: ts.Program): PrivateDeclarationsAnalyses { | ||
| const rootFiles = this.getRootFiles(program); | ||
| return this.getPrivateDeclarations(rootFiles, this.referencesRegistry.getDeclarationMap()); | ||
| } | ||
|
|
||
| private getRootFiles(program: ts.Program): ts.SourceFile[] { | ||
| return program.getRootFileNames().map(f => program.getSourceFile(f)).filter(isDefined); | ||
| } | ||
|
|
||
| private getPrivateDeclarations( | ||
| rootFiles: ts.SourceFile[], | ||
| declarations: Map<ts.Identifier, Declaration>): PrivateDeclarationsAnalyses { | ||
| const privateDeclarations: Map<ts.Identifier, Declaration> = new Map(declarations); | ||
| rootFiles.forEach(f => { | ||
| const exports = this.host.getExportsOfModule(f); | ||
| if (exports) { | ||
| exports.forEach((declaration, exportedName) => { | ||
| if (hasNameIdentifier(declaration.node) && declaration.node.name.text === exportedName) { | ||
| privateDeclarations.delete(declaration.node.name); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| return Array.from(privateDeclarations.keys()).map(id => { | ||
| const from = id.getSourceFile().fileName; | ||
| const declaration = privateDeclarations.get(id) !; | ||
| const dtsDeclaration = this.host.getDtsDeclarationOfClass(declaration.node); | ||
| const dtsFrom = dtsDeclaration && dtsDeclaration.getSourceFile().fileName; | ||
| return {identifier: id.text, from, dtsFrom}; | ||
| }); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,8 @@ import * as ts from 'typescript'; | |
|
|
||
| import {ClassMember, ClassMemberKind, CtorParameter, Decorator, Import} from '../../../ngtsc/host'; | ||
| import {TypeScriptReflectionHost, reflectObjectLiteral} from '../../../ngtsc/metadata'; | ||
| import {findAll, getNameText, getOriginalSymbol, isDefined} from '../utils'; | ||
| import {BundleProgram} from '../packages/bundle_program'; | ||
| import {findAll, getNameText, isDefined} from '../utils'; | ||
|
|
||
| import {DecoratedClass} from './decorated_class'; | ||
| import {NgccReflectionHost, PRE_R3_MARKER, SwitchableVariableDeclaration, isSwitchableVariableDeclaration} from './ngcc_host'; | ||
|
|
@@ -49,13 +50,9 @@ export const CONSTRUCTOR_PARAMS = 'ctorParameters' as ts.__String; | |
| */ | ||
| export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements NgccReflectionHost { | ||
| protected dtsClassMap: Map<string, ts.ClassDeclaration>|null; | ||
| constructor( | ||
| protected isCore: boolean, checker: ts.TypeChecker, dtsRootFileName?: string, | ||
| dtsProgram?: ts.Program|null) { | ||
| constructor(protected isCore: boolean, checker: ts.TypeChecker, dts?: BundleProgram|null) { | ||
| super(checker); | ||
| this.dtsClassMap = (dtsRootFileName && dtsProgram) ? | ||
| this.computeDtsClassMap(dtsRootFileName, dtsProgram) : | ||
| null; | ||
| this.dtsClassMap = dts && this.computeDtsClassMap(dts.path, dts.program) || null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -356,12 +353,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N | |
| throw new Error( | ||
| `Cannot get the dts file for a class declaration that has no indetifier: ${declaration.getText()} in ${declaration.getSourceFile().fileName}`); | ||
| } | ||
| const dtsDeclaration = this.dtsClassMap.get(declaration.name.text); | ||
| if (!dtsDeclaration) { | ||
| throw new Error( | ||
| `Unable to find matching typings (.d.ts) declaration for ${declaration.name.text} in ${declaration.getSourceFile().fileName}`); | ||
| } | ||
| return dtsDeclaration; | ||
| return this.dtsClassMap.get(declaration.name.text) || null; | ||
| } | ||
| } | ||
| return null; | ||
|
|
@@ -853,7 +845,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N | |
| } | ||
|
|
||
| /** | ||
| * Get the parmeter type and decorators for a class where the information is stored on | ||
| * Get the parameter type and decorators for a class where the information is stored on | ||
| * in calls to `__decorate` helpers. | ||
| * | ||
| * Reflect over the helpers to find the decorators and types about each of | ||
|
|
@@ -978,7 +970,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N | |
| * Test whether a decorator was imported from `@angular/core`. | ||
| * | ||
| * Is the decorator: | ||
| * * externally imported from `@angulare/core`? | ||
| * * externally imported from `@angular/core`? | ||
| * * the current hosted program is actually `@angular/core` and | ||
| * - relatively internally imported; or | ||
| * - not imported, from the current file. | ||
|
|
@@ -993,31 +985,37 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Extract all the class declarations from the dtsTypings program, storing them in a map | ||
| * where the key is the declared name of the class and the value is the declaration itself. | ||
| * | ||
| * It is possible for there to be multiple class declarations with the same local name. | ||
| * Only the first declaration with a given name is added to the map; subsequent classes will be | ||
| * ignored. | ||
| * | ||
| * We are most interested in classes that are publicly exported from the entry point, so these are | ||
| * added to the map first, to ensure that they are not ignored. | ||
| * | ||
| * @param dtsRootFileName The filename of the entry-point to the `dtsTypings` program. | ||
| * @param dtsProgram The program containing all the typings files. | ||
| * @returns a map of class names to class declarations. | ||
| */ | ||
| protected computeDtsClassMap(dtsRootFileName: string, dtsProgram: ts.Program): | ||
| Map<string, ts.ClassDeclaration> { | ||
| const dtsClassMap = new Map<string, ts.ClassDeclaration>(); | ||
| const checker = dtsProgram.getTypeChecker(); | ||
| const dtsRootFile = dtsProgram.getSourceFile(dtsRootFileName); | ||
| const rootModule = dtsRootFile && checker.getSymbolAtLocation(dtsRootFile); | ||
| const moduleExports = rootModule && checker.getExportsOfModule(rootModule); | ||
| if (moduleExports) { | ||
| moduleExports.forEach(exportedSymbol => { | ||
| if (exportedSymbol.flags & ts.SymbolFlags.Alias) { | ||
| exportedSymbol = checker.getAliasedSymbol(exportedSymbol); | ||
| } | ||
| const declaration = exportedSymbol.declarations[0]; | ||
| if (declaration && ts.isClassDeclaration(declaration)) { | ||
| const name = exportedSymbol.name; | ||
| const previousDeclaration = dtsClassMap.get(name); | ||
| if (previousDeclaration && previousDeclaration !== declaration) { | ||
| console.warn( | ||
| `Ambiguous class name ${name} in typings files: ${previousDeclaration.getSourceFile().fileName} and ${declaration.getSourceFile().fileName}`); | ||
| } else { | ||
| dtsClassMap.set(name, declaration); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // First add all the classes that are publicly exported from the entry-point | ||
| const rootFile = dtsProgram.getSourceFile(dtsRootFileName); | ||
| if (!rootFile) { | ||
| throw new Error(`The given file ${dtsRootFileName} is not part of the typings program.`); | ||
| } | ||
| collectExportedClasses(checker, dtsClassMap, rootFile); | ||
|
|
||
| // Now add any additional classes that are exported from individual dts files, | ||
| // but are not publicly exported from the entry-point. | ||
| dtsProgram.getSourceFiles().forEach( | ||
| sourceFile => { collectExportedClasses(checker, dtsClassMap, sourceFile); }); | ||
| return dtsClassMap; | ||
| } | ||
| } | ||
|
|
@@ -1151,3 +1149,28 @@ function getFarLeftIdentifier(propertyAccess: ts.PropertyAccessExpression): ts.I | |
| } | ||
| return ts.isIdentifier(propertyAccess.expression) ? propertyAccess.expression : null; | ||
| } | ||
|
|
||
| /** | ||
| * Search a source file for exported classes, storing them in the provided `dtsClassMap`. | ||
| * @param checker The typechecker for the source program. | ||
| * @param dtsClassMap The map in which to store the collected exported classes. | ||
| * @param srcFile The source file to search for exported classes. | ||
| */ | ||
| function collectExportedClasses( | ||
| checker: ts.TypeChecker, dtsClassMap: Map<string, ts.ClassDeclaration>, | ||
| srcFile: ts.SourceFile): void { | ||
| const srcModule = srcFile && checker.getSymbolAtLocation(srcFile); | ||
| const moduleExports = srcModule && checker.getExportsOfModule(srcModule); | ||
| if (moduleExports) { | ||
| moduleExports.forEach(exportedSymbol => { | ||
| if (exportedSymbol.flags & ts.SymbolFlags.Alias) { | ||
| exportedSymbol = checker.getAliasedSymbol(exportedSymbol); | ||
| } | ||
| const declaration = exportedSymbol.valueDeclaration; | ||
| const name = exportedSymbol.name; | ||
| if (declaration && ts.isClassDeclaration(declaration) && !dtsClassMap.has(name)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it legal even to have two classes exported under the same name? How would you import one vs the other? I don't think the I think we should crash if this happens, instead of silently ignoring other classes under the same name. Actually, I think this can happen if you have both a type and a value with the same name, e.g: We should filter out symbols which only resolve to types (don't resolve to a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can happen because we are not only looking at exports in It is true that only one could ever be exported publicly from the entry-point, but this PR has to deal with "internal" classes too.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should still do something other than silently ignore when there's more than one class with the same name. Can we store an array of them, or put a special marker in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two scenarios, we should deal with:
In most cases it is the public one we are after, right? So should we just ignore additional internal matching names? But if we are trying to create an export for an internal class and we have multiple classes with the same name, then I think this should probably error, since we don't know which we are supposed to be exporting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I think that the best solution is for |
||
| dtsClassMap.set(name, declaration); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, being less strict here resolves an issue for me as well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I think I am done with refactoring this. Ready for a proper review.