From 772b24ccc36796408ccee46c0a0ce50b51ab05a4 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Fri, 22 Feb 2019 18:06:25 -0800 Subject: [PATCH] fix(ivy): avoid missing imports for types that can be represented as values (#28941) Prior to this change, TypeScript stripped out some imports in case we reference a type that can be represented as a value (for ex. classes). This fix ensures that we use correct symbol identifier, which makes TypeScript retain the necessary import statements. PR Close #28941 --- .../src/ngtsc/reflection/src/typescript.ts | 28 ++++-- packages/compiler-cli/test/ngtsc/env.ts | 1 + .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 94 +++++++++++++++++++ 3 files changed, 116 insertions(+), 7 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index 519e767138..708b953965 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -68,16 +68,30 @@ export class TypeScriptReflectionHost implements ReflectionHost { } // It's not possible to get a value expression if the parameter doesn't even have a type. - if (typeNode) { + if (typeNode && ts.isTypeReferenceNode(typeNode)) { // It's only valid to convert a type reference to a value reference if the type actually has // a value declaration associated with it. - let type: ts.Type|null = this.checker.getTypeFromTypeNode(typeNode); + let symbol: ts.Symbol|undefined = this.checker.getSymbolAtLocation(typeNode.typeName); - if (type && type.symbol !== undefined && type.symbol.valueDeclaration !== undefined) { - // The type points to a valid value declaration. Rewrite the TypeReference into an - // Expression - // which references the value pointed to by the TypeReference, if possible. - typeValueExpr = typeNodeToValueExpr(typeNode); + if (symbol !== undefined) { + let resolvedSymbol = symbol; + if (symbol.flags & ts.SymbolFlags.Alias) { + resolvedSymbol = this.checker.getAliasedSymbol(symbol); + } + if (resolvedSymbol.valueDeclaration !== undefined) { + // The type points to a valid value declaration. Rewrite the TypeReference into an + // Expression which references the value pointed to by the TypeReference, if possible. + const firstDecl = symbol.declarations && symbol.declarations[0]; + if (firstDecl && ts.isImportSpecifier(firstDecl)) { + // Making sure TS produces the necessary imports in case a symbol was declared in a + // different script and imported. To do that we check symbol's first declaration and + // if it's an import - use its identifier. The `Identifier` from the `ImportSpecifier` + // knows it could be a value reference, and will emit as one if needed. + typeValueExpr = ts.updateIdentifier(firstDecl.name); + } else { + typeValueExpr = typeNodeToValueExpr(typeNode); + } + } } } diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts index 9b5d29b25a..6ce40e673c 100644 --- a/packages/compiler-cli/test/ngtsc/env.ts +++ b/packages/compiler-cli/test/ngtsc/env.ts @@ -56,6 +56,7 @@ export class NgtscTestEnvironment { env.write('tsconfig-base.json', `{ "compilerOptions": { + "emitDecoratorMetadata": true, "experimentalDecorators": true, "skipLibCheck": true, "noImplicitAny": true, diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 7fd1a4547e..f0ada1735e 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -26,6 +26,9 @@ const contentQueryRegExp = (predicate: string, descend: boolean, ref?: string): return new RegExp(`i0\\.ɵcontentQuery\\(dirIndex, ${predicate}, ${descend}, ${maybeRef}\\)`); }; +const setClassMetadataRegExp = (expectedType: string): RegExp => + new RegExp(`setClassMetadata(.*?${expectedType}.*?)`); + describe('ngtsc behavioral tests', () => { let env !: NgtscTestEnvironment; @@ -1843,6 +1846,97 @@ describe('ngtsc behavioral tests', () => { expect(jsContents).toContain('ɵsetClassMetadata(TestPipe, '); }); + it('should use imported types in setClassMetadata if they can be represented as values', () => { + env.tsconfig({}); + + env.write(`types.ts`, ` + export class MyTypeA {} + export class MyTypeB {} + `); + env.write(`test.ts`, ` + import {Component, Inject, Injectable} from '@angular/core'; + import {MyTypeA, MyTypeB} from './types'; + + @Injectable({providedIn: 'root'}) + export class SomeService { + constructor(arg: MyTypeA) {} + } + + @Component({ + selector: 'some-comp', + template: '...', + }) + export class SomeComp { + constructor(@Inject('arg-token') arg: MyTypeB) {} + } + `); + + env.driveMain(); + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain(`import { MyTypeA, MyTypeB } from './types';`); + expect(jsContents).toMatch(setClassMetadataRegExp('type: MyTypeA')); + expect(jsContents).toMatch(setClassMetadataRegExp('type: MyTypeB')); + }); + + it('should use imported types in setClassMetadata if they can be represented as values and imported as `* as foo`', + () => { + env.tsconfig({}); + + env.write(`types.ts`, ` + export class MyTypeA {} + export class MyTypeB {} + `); + env.write(`test.ts`, ` + import {Component, Inject, Injectable} from '@angular/core'; + import * as types from './types'; + + @Injectable({providedIn: 'root'}) + export class SomeService { + constructor(arg: types.MyTypeA) {} + } + + @Component({ + selector: 'some-comp', + template: '...', + }) + export class SomeComp { + constructor(@Inject('arg-token') arg: types.MyTypeB) {} + } + `); + + env.driveMain(); + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).toContain(`import * as types from './types';`); + expect(jsContents).toMatch(setClassMetadataRegExp('type: types.MyTypeA')); + expect(jsContents).toMatch(setClassMetadataRegExp('type: types.MyTypeB')); + }); + + it('should use `undefined` in setClassMetadata if types can\'t be represented as values', () => { + env.tsconfig({}); + + env.write(`types.ts`, ` + export type MyType = Map; + `); + env.write(`test.ts`, ` + import {Component, Inject, Injectable} from '@angular/core'; + import {MyType} from './types'; + + @Component({ + selector: 'some-comp', + template: '...', + }) + export class SomeComp { + constructor(@Inject('arg-token') arg: MyType) {} + } + `); + + env.driveMain(); + const jsContents = trim(env.getContents('test.js')); + expect(jsContents).not.toContain(`import { MyType } from './types';`); + // Note: `type: undefined` below, since MyType can't be represented as a value + expect(jsContents).toMatch(setClassMetadataRegExp('type: undefined')); + }); + it('should not throw in case whitespaces and HTML comments are present inside ', () => { env.tsconfig();