From ed7aa1c3e57f5462d8006991f0bd8af9bec4d925 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 24 Jul 2018 16:10:15 -0700 Subject: [PATCH] fix(ivy): force new imports for .d.ts files (#25080) When ngtsc encounters a reference to a type (for example, a Component type listed in an NgModule declarations array), it traces the import of that type and attempts to determine the best way to refer to it. In the event the type is defined in the same file where a reference is being generated, the identifier of the type is used. If the type was imported, ngtsc has a choice. It can use the identifier from the original import, or it can write a new import to the module where the type came from. ngtsc has a bug currently when it elects to rely on the user's import. When writing a .d.ts file, the user's import may have been elided as the type was not referred to from the type side of the program. Thus, in .d.ts files ngtsc must always assume the import may not exist, and generate a new one. In .js output the import is guaranteed to still exist, so it's preferable for ngtsc to continue using the existing import if one is available. This commit changes how @angular/compiler writes type definitions, and allows it to use a different expression to write a type definition than is used to write the value. This allows ngtsc to specify that types in type definitions should always be imported. A corresponding change to the staticallyResolve() Reference system allows the choice of which type of import to use when generating an Expression from a Reference. PR Close #25080 --- .../src/ngtsc/annotations/src/ng_module.ts | 8 +-- .../ngtsc/annotations/src/selector_scope.ts | 4 +- .../src/ngtsc/annotations/src/util.ts | 13 ++-- .../compiler-cli/src/ngtsc/metadata/index.ts | 2 +- .../src/ngtsc/metadata/src/resolver.ts | 41 +++++++++++-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 59 +++++++++++++++++++ packages/compiler/src/compiler.ts | 1 + .../src/render3/r3_module_compiler.ts | 18 +++--- packages/compiler/src/render3/util.ts | 5 ++ packages/core/src/render3/jit/module.ts | 19 ++++-- 10 files changed, 136 insertions(+), 34 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 68f6fd8744..1421362c39 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -14,7 +14,7 @@ import {Reference, ResolvedValue, reflectObjectLiteral, staticallyResolve} from import {AnalysisOutput, CompileResult, DecoratorHandler} from '../../transform'; import {SelectorScopeRegistry} from './selector_scope'; -import {getConstructorDependencies, isAngularCore, referenceToExpression, unwrapExpression} from './util'; +import {getConstructorDependencies, isAngularCore, toR3Reference, unwrapExpression} from './util'; export interface NgModuleAnalysis { ngModuleDef: R3NgModuleMetadata; @@ -87,9 +87,9 @@ export class NgModuleDecoratorHandler implements DecoratorHandler referenceToExpression(decl, context)), - exports: exports.map(exp => referenceToExpression(exp, context)), - imports: imports.map(imp => referenceToExpression(imp, context)), + declarations: declarations.map(decl => toR3Reference(decl, context)), + exports: exports.map(exp => toR3Reference(exp, context)), + imports: imports.map(imp => toR3Reference(imp, context)), emitInline: false, }; diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts b/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts index 997c60c96e..8927f24c84 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts @@ -13,7 +13,7 @@ import {ReflectionHost} from '../../host'; import {AbsoluteReference, Reference, reflectTypeEntityToDeclaration} from '../../metadata'; import {reflectIdentifierOfDeclaration, reflectNameOfDeclaration} from '../../metadata/src/reflector'; -import {referenceToExpression} from './util'; +import {toR3Reference} from './util'; @@ -384,7 +384,7 @@ function convertReferenceMap( map: Map, context: ts.SourceFile): Map { return new Map(Array.from(map.entries()).map(([selector, ref]): [ string, Expression - ] => [selector, referenceToExpression(ref, context)])); + ] => [selector, toR3Reference(ref, context).value])); } function convertScopeToExpressions( diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts index 8cd5ea2caf..45cc51d530 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/util.ts @@ -6,11 +6,11 @@ * found in the LICENSE file at https://angular.io/license */ -import {Expression, R3DependencyMetadata, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler'; +import {Expression, R3DependencyMetadata, R3Reference, R3ResolvedDependencyType, WrappedNodeExpr} from '@angular/compiler'; import * as ts from 'typescript'; import {Decorator, ReflectionHost} from '../../host'; -import {AbsoluteReference, Reference} from '../../metadata'; +import {AbsoluteReference, ImportMode, Reference} from '../../metadata'; export function getConstructorDependencies( clazz: ts.ClassDeclaration, reflector: ReflectionHost, @@ -79,12 +79,13 @@ export function getConstructorDependencies( return useType; } -export function referenceToExpression(ref: Reference, context: ts.SourceFile): Expression { - const exp = ref.toExpression(context); - if (exp === null) { +export function toR3Reference(ref: Reference, context: ts.SourceFile): R3Reference { + const value = ref.toExpression(context, ImportMode.UseExistingImport); + const type = ref.toExpression(context, ImportMode.ForceNewImport); + if (value === null || type === null) { throw new Error(`Could not refer to ${ts.SyntaxKind[ref.node.kind]}`); } - return exp; + return {value, type}; } export function isAngularCore(decorator: Decorator): boolean { diff --git a/packages/compiler-cli/src/ngtsc/metadata/index.ts b/packages/compiler-cli/src/ngtsc/metadata/index.ts index dd4a581356..3178a1376e 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/index.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/index.ts @@ -7,4 +7,4 @@ */ export {TypeScriptReflectionHost, filterToMembersWithDecorator, findMember, reflectObjectLiteral, reflectTypeEntityToDeclaration} from './src/reflector'; -export {AbsoluteReference, Reference, ResolvedValue, isDynamicValue, staticallyResolve} from './src/resolver'; +export {AbsoluteReference, ImportMode, Reference, ResolvedValue, isDynamicValue, staticallyResolve} from './src/resolver'; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts b/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts index 698d3ab15d..c523822c57 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts @@ -78,6 +78,11 @@ export interface ResolvedValueArray extends Array {} */ type Scope = Map; +export enum ImportMode { + UseExistingImport, + ForceNewImport, +} + /** * A reference to a `ts.Node`. * @@ -99,7 +104,7 @@ export abstract class Reference { * This could be a local variable reference, if the symbol is imported, or it could be a new * import if needed. */ - abstract toExpression(context: ts.SourceFile): Expression|null; + abstract toExpression(context: ts.SourceFile, importMode?: ImportMode): Expression|null; abstract withIdentifier(identifier: ts.Identifier): Reference; } @@ -115,7 +120,7 @@ export class NodeReference extends Reference { toExpression(context: ts.SourceFile): null { return null; } - withIdentifier(identifier: ts.Identifier): NodeReference { return this; } + withIdentifier(identifier: ts.Identifier): NodeReference { return this; } } /** @@ -128,8 +133,20 @@ export class ResolvedReference extends Reference readonly expressable = true; - toExpression(context: ts.SourceFile): Expression { - if (ts.getOriginalNode(context) === ts.getOriginalNode(this.identifier).getSourceFile()) { + toExpression(context: ts.SourceFile, importMode: ImportMode = ImportMode.UseExistingImport): + Expression { + let compareCtx: ts.Node|null = null; + switch (importMode) { + case ImportMode.UseExistingImport: + compareCtx = this.identifier; + break; + case ImportMode.ForceNewImport: + compareCtx = this.node; + break; + default: + throw new Error(`Unsupported ImportMode: ${ImportMode[importMode]}`); + } + if (ts.getOriginalNode(context) === ts.getOriginalNode(compareCtx).getSourceFile()) { return new WrappedNodeExpr(this.identifier); } else { // Relative import from context -> this.node.getSourceFile(). @@ -175,8 +192,20 @@ export class AbsoluteReference extends Reference { readonly expressable = true; - toExpression(context: ts.SourceFile): Expression { - if (ts.getOriginalNode(context) === ts.getOriginalNode(this.identifier).getSourceFile()) { + toExpression(context: ts.SourceFile, importMode: ImportMode = ImportMode.UseExistingImport): + Expression { + let compareCtx: ts.Node|null = null; + switch (importMode) { + case ImportMode.UseExistingImport: + compareCtx = this.identifier; + break; + case ImportMode.ForceNewImport: + compareCtx = this.node; + break; + default: + throw new Error(`Unsupported ImportMode: ${ImportMode[importMode]}`); + } + if (ts.getOriginalNode(context) === ts.getOriginalNode(compareCtx).getSourceFile()) { return new WrappedNodeExpr(this.identifier); } else { return new ExternalExpr(new ExternalReference(this.moduleName, this.symbolName)); diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 352bcb4e02..be3cc935d8 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -246,6 +246,65 @@ describe('ngtsc behavioral tests', () => { expect(dtsContents).toContain('static ngInjectorDef: i0.ɵInjectorDef'); }); + it('should compile NgModules with references to local components', () => { + writeConfig(); + write('test.ts', ` + import {NgModule} from '@angular/core'; + import {Foo} from './foo'; + + @NgModule({ + declarations: [Foo], + }) + export class FooModule {} + `); + write('foo.ts', ` + import {Component} from '@angular/core'; + @Component({selector: 'foo', template: ''}) + export class Foo {} + `); + + const exitCode = main(['-p', basePath], errorSpy); + expect(errorSpy).not.toHaveBeenCalled(); + expect(exitCode).toBe(0); + + const jsContents = getContents('test.js'); + const dtsContents = getContents('test.d.ts'); + + expect(jsContents).toContain('import { Foo } from \'./foo\';'); + expect(jsContents).not.toMatch(/as i[0-9] from '.\/foo'/); + expect(dtsContents).toContain('as i1 from \'./foo\';'); + }); + + it('should compile NgModules with references to absolute components', () => { + writeConfig(); + write('test.ts', ` + import {NgModule} from '@angular/core'; + import {Foo} from 'foo'; + + @NgModule({ + declarations: [Foo], + }) + export class FooModule {} + `); + write('node_modules/foo/index.d.ts', ` + import * as i0 from '@angular/core'; + export class Foo { + static ngComponentDef: i0.ɵComponentDef; + } + `); + + const exitCode = main(['-p', basePath], errorSpy); + expect(errorSpy).not.toHaveBeenCalled(); + expect(exitCode).toBe(0); + + const jsContents = getContents('test.js'); + const dtsContents = getContents('test.d.ts'); + + expect(jsContents).toContain('import { Foo } from \'foo\';'); + expect(jsContents).not.toMatch(/as i[0-9] from 'foo'/); + expect(dtsContents).toContain('as i1 from \'foo\';'); + }); + it('should compile Pipes without errors', () => { writeConfig(); write('test.ts', ` diff --git a/packages/compiler/src/compiler.ts b/packages/compiler/src/compiler.ts index 2f81a1cd85..ca762310af 100644 --- a/packages/compiler/src/compiler.ts +++ b/packages/compiler/src/compiler.ts @@ -86,5 +86,6 @@ export {R3DependencyMetadata, R3FactoryMetadata, R3ResolvedDependencyType} from export {compileInjector, compileNgModule, R3InjectorMetadata, R3NgModuleMetadata} from './render3/r3_module_compiler'; export {compilePipeFromMetadata, R3PipeMetadata} from './render3/r3_pipe_compiler'; export {makeBindingParser, parseTemplate} from './render3/view/template'; +export {R3Reference} from './render3/util'; export {compileComponentFromMetadata, compileDirectiveFromMetadata, parseHostBindings} from './render3/view/compiler'; // This file only reexports content of the `src` folder. Keep it that way. \ No newline at end of file diff --git a/packages/compiler/src/render3/r3_module_compiler.ts b/packages/compiler/src/render3/r3_module_compiler.ts index e9938bf5a5..a1153a0ef7 100644 --- a/packages/compiler/src/render3/r3_module_compiler.ts +++ b/packages/compiler/src/render3/r3_module_compiler.ts @@ -15,7 +15,7 @@ import {OutputContext} from '../util'; import {R3DependencyMetadata, compileFactoryFunction} from './r3_factory'; import {Identifiers as R3} from './r3_identifiers'; -import {convertMetaToOutput, mapToMapExpression} from './util'; +import {R3Reference, convertMetaToOutput, mapToMapExpression} from './util'; export interface R3NgModuleDef { expression: o.Expression; @@ -40,17 +40,17 @@ export interface R3NgModuleMetadata { /** * An array of expressions representing the directives and pipes declared by the module. */ - declarations: o.Expression[]; + declarations: R3Reference[]; /** * An array of expressions representing the imports of the module. */ - imports: o.Expression[]; + imports: R3Reference[]; /** * An array of expressions representing the exports of the module. */ - exports: o.Expression[]; + exports: R3Reference[]; /** * Whether to emit the selector scope values (declarations, imports, exports) inline into the @@ -68,9 +68,9 @@ export function compileNgModule(meta: R3NgModuleMetadata): R3NgModuleDef { const expression = o.importExpr(R3.defineNgModule).callFn([mapToMapExpression({ type: moduleType, bootstrap: o.literalArr(bootstrap), - declarations: o.literalArr(declarations), - imports: o.literalArr(imports), - exports: o.literalArr(exports), + declarations: o.literalArr(declarations.map(ref => ref.value)), + imports: o.literalArr(imports.map(ref => ref.value)), + exports: o.literalArr(exports.map(ref => ref.value)), })]); const type = new o.ExpressionType(o.importExpr(R3.NgModuleDef, [ @@ -148,7 +148,7 @@ function accessExportScope(module: o.Expression): o.Expression { return new o.ReadPropExpr(selectorScope, 'exported'); } -function tupleTypeOf(exp: o.Expression[]): o.Type { - const types = exp.map(type => o.typeofExpr(type)); +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; } \ No newline at end of file diff --git a/packages/compiler/src/render3/util.ts b/packages/compiler/src/render3/util.ts index ae619ef2da..c006c298f5 100644 --- a/packages/compiler/src/render3/util.ts +++ b/packages/compiler/src/render3/util.ts @@ -47,3 +47,8 @@ export function typeWithParameters(type: o.Expression, numParams: number): o.Exp } return o.expressionType(type, null, params); } + +export interface R3Reference { + value: o.Expression; + type: o.Expression; +} diff --git a/packages/core/src/render3/jit/module.ts b/packages/core/src/render3/jit/module.ts index 823f30a001..91e1297306 100644 --- a/packages/core/src/render3/jit/module.ts +++ b/packages/core/src/render3/jit/module.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Expression, R3InjectorMetadata, R3NgModuleMetadata, WrappedNodeExpr, compileInjector, compileNgModule as compileR3NgModule, jitExpression} from '@angular/compiler'; +import {Expression, R3InjectorMetadata, R3NgModuleMetadata, R3Reference, WrappedNodeExpr, compileInjector, compileNgModule as compileR3NgModule, jitExpression} from '@angular/compiler'; import {ModuleWithProviders, NgModule, NgModuleDefInternal, NgModuleTransitiveScopes} from '../../metadata/ng_module'; import {Type} from '../../type'; @@ -28,11 +28,13 @@ export function compileNgModule(type: Type, ngModule: NgModule): void { const meta: R3NgModuleMetadata = { type: wrap(type), bootstrap: flatten(ngModule.bootstrap || EMPTY_ARRAY).map(wrap), - declarations: declarations.map(wrap), - imports: - flatten(ngModule.imports || EMPTY_ARRAY).map(expandModuleWithProviders).map(wrap), - exports: - flatten(ngModule.exports || EMPTY_ARRAY).map(expandModuleWithProviders).map(wrap), + declarations: declarations.map(wrapReference), + imports: flatten(ngModule.imports || EMPTY_ARRAY) + .map(expandModuleWithProviders) + .map(wrapReference), + exports: flatten(ngModule.exports || EMPTY_ARRAY) + .map(expandModuleWithProviders) + .map(wrapReference), emitInline: true, }; const res = compileR3NgModule(meta); @@ -210,6 +212,11 @@ function wrap(value: Type): Expression { return new WrappedNodeExpr(value); } +function wrapReference(value: Type): R3Reference { + const wrapped = wrap(value); + return {value: wrapped, type: wrapped}; +} + function isModuleWithProviders(value: any): value is ModuleWithProviders { return (value as{ngModule?: any}).ngModule !== undefined; }