Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions integration/ngcc/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@


"@angular/animations@file:../../dist/packages-dist/animations":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

Expand All @@ -17,14 +17,14 @@
parse5 "^5.0.0"

"@angular/common@file:../../dist/packages-dist/common":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

"@angular/compiler-cli@file:../../dist/packages-dist/compiler-cli":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
canonical-path "0.0.2"
canonical-path "1.0.0"
chokidar "^1.4.2"
convert-source-map "^1.5.1"
dependency-graph "^0.7.2"
Expand All @@ -33,25 +33,26 @@
reflect-metadata "^0.1.2"
shelljs "^0.8.1"
source-map "^0.6.1"
tslib "^1.9.0"
yargs "9.0.1"

"@angular/compiler@file:../../dist/packages-dist/compiler":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

"@angular/core@file:../../dist/packages-dist/core":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

"@angular/forms@file:../../dist/packages-dist/forms":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

"@angular/http@file:../../dist/packages-dist/http":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

Expand All @@ -65,17 +66,17 @@
parse5 "^5.0.0"

"@angular/platform-browser-dynamic@file:../../dist/packages-dist/platform-browser-dynamic":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

"@angular/platform-browser@file:../../dist/packages-dist/platform-browser":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

"@angular/router@file:../../dist/packages-dist/router":
version "7.1.0-beta.1"
version "7.1.0"
dependencies:
tslib "^1.9.0"

Expand Down Expand Up @@ -533,10 +534,10 @@ camelcase@^4.1.0:
resolved "https://registry.yarnpkg.com/camelcase/-/camelcase-4.1.0.tgz#d545635be1e33c542649c69173e5de6acfae34dd"
integrity sha1-1UVjW+HjPFQmScaRc+Xeas+uNN0=

canonical-path@0.0.2:
version "0.0.2"
resolved "https://registry.yarnpkg.com/canonical-path/-/canonical-path-0.0.2.tgz#e31eb937a8c93ee2a01df1839794721902874574"
integrity sha1-4x65N6jJPuKgHfGDl5RyGQKHRXQ=
canonical-path@1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/canonical-path/-/canonical-path-1.0.0.tgz#fcb470c23958def85081856be7a86e904f180d1d"
integrity sha512-feylzsbDxi1gPZ1IjystzIQZagYYLvfKrSuygUCgf7z6x790VEzze5QEkdSV1U58RA7Hi0+v6fv4K54atOzATg==

caseless@~0.12.0:
version "0.12.0"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ import {ConstantPool} from '@angular/compiler';
import * as fs from 'fs';
import * as ts from 'typescript';

import {BaseDefDecoratorHandler, ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ResourceLoader, SelectorScopeRegistry} from '../../../ngtsc/annotations';
import {BaseDefDecoratorHandler, ComponentDecoratorHandler, DirectiveDecoratorHandler, InjectableDecoratorHandler, NgModuleDecoratorHandler, PipeDecoratorHandler, ReferencesRegistry, ResourceLoader, SelectorScopeRegistry} from '../../../ngtsc/annotations';
import {CompileResult, DecoratorHandler} from '../../../ngtsc/transform';

import {DecoratedClass} from '../host/decorated_class';
import {NgccReflectionHost} from '../host/ngcc_host';
import {isDefined} from '../utils';
Expand Down Expand Up @@ -63,13 +62,15 @@ export class DecorationAnalyzer {
this.rootDirs),
new DirectiveDecoratorHandler(this.typeChecker, this.host, this.scopeRegistry, this.isCore),
new InjectableDecoratorHandler(this.host, this.isCore),
new NgModuleDecoratorHandler(this.typeChecker, this.host, this.scopeRegistry, this.isCore),
new NgModuleDecoratorHandler(
this.typeChecker, this.host, this.scopeRegistry, this.referencesRegistry, this.isCore),
new PipeDecoratorHandler(this.typeChecker, this.host, this.scopeRegistry, this.isCore),
];

constructor(
private typeChecker: ts.TypeChecker, private host: NgccReflectionHost,
private rootDirs: string[], private isCore: boolean) {}
private referencesRegistry: ReferencesRegistry, private rootDirs: string[],
private isCore: boolean) {}

/**
* Analyze a program to find all the decorated files should be transformed.
Expand Down
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};
});
}
}
93 changes: 58 additions & 35 deletions packages/compiler-cli/src/ngcc/src/host/esm2015_host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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;
Copy link
Copy Markdown
Member

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 👍

Copy link
Copy Markdown
Contributor Author

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.

}
}
return null;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 TypeChecker will ever give such a result back.

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:

export interface Foo {}
export const Foo = ...;

We should filter out symbols which only resolve to types (don't resolve to a Symbol with a valueDeclaration).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen because we are not only looking at exports in srcFile but also exports from other source files too. collectExportedClasses gets called for each source file in the program, and each one adds to the dtsMap.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 Map that will cause an error if we try to publicly export one of these?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two scenarios, we should deal with:

  1. There is both a public and one or more internal exports with the same name.
  2. There is no public but more than one internal export with the same name.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think that the best solution is for getDtsDeclarationOfClass to return an array of possible declarations, with the public one (if any) first. The user of getDtsDeclarationOfClass then has to decide whether having more than one result is an error or not.

dtsClassMap.set(name, declaration);
}
});
}
}
Loading