From 29b8666b10a963fa0f7bf772e563b21844a7f6b7 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 18 Nov 2019 11:53:25 -0800 Subject: [PATCH] fix(ngcc): properly detect origin of constructor param types (#33901) The ReflectionHost supports enumeration of constructor parameters, and one piece of information it returns describes the origin of the parameter's type. Parameter types come in two flavors: local (the type is not imported from anywhere) or non-local (the type comes via an import). ngcc incorrectly classified all type parameters as 'local', because in the source files that ngcc processes the type parameter is a real ts.Identifer. However, that identifier may still have come from an import and thus might be non-local. This commit changes ngcc's ReflectionHost(s) to properly recognize and report these non-local type references. Fixes #33677 PR Close #33901 --- .../ngcc/src/host/esm2015_host.ts | 30 ++++++++++++++++--- .../ngcc/test/host/esm2015_host_spec.ts | 23 +++++--------- .../ngcc/test/host/umd_host_spec.ts | 3 -- packages/compiler-cli/ngcc/test/host/util.ts | 21 +++++++++---- 4 files changed, 50 insertions(+), 27 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index f926f216d8..ef6dd09114 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -9,7 +9,7 @@ import * as ts from 'typescript'; import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; -import {ClassDeclaration, ClassMember, ClassMemberKind, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection'; +import {ClassDeclaration, ClassMember, ClassMemberKind, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, TypeValueReference, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection'; import {isWithinPackage} from '../analysis/util'; import {Logger} from '../logging/logger'; import {BundleProgram} from '../packages/bundle_program'; @@ -1357,12 +1357,34 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N constructorParamInfo[index] : {decorators: null, typeExpression: null}; const nameNode = node.name; + + let typeValueReference: TypeValueReference|null = null; + if (typeExpression !== null) { + // `typeExpression` is an expression in a "type" context. Resolve it to a declared value. + // Either it's a reference to an imported type, or a type declared locally. Distinguish the + // two cases with `getDeclarationOfExpression`. + const decl = this.getDeclarationOfExpression(typeExpression); + if (decl !== null && decl.node !== null && decl.viaModule !== null && + isNamedDeclaration(decl.node)) { + typeValueReference = { + local: false, + valueDeclaration: decl.node, + moduleName: decl.viaModule, + name: decl.node.name.text, + }; + } else { + typeValueReference = { + local: true, + expression: typeExpression, + defaultImportStatement: null, + }; + } + } + return { name: getNameText(nameNode), nameNode, - typeValueReference: typeExpression !== null ? - {local: true as true, expression: typeExpression, defaultImportStatement: null} : - null, + typeValueReference, typeNode: null, decorators }; }); diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts index 43ce43a19d..4934a4fac2 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts @@ -54,11 +54,10 @@ runInEachFileSystem(() => { SOME_DIRECTIVE_FILE = { name: _('/some_directive.js'), contents: ` - import { Directive, Inject, InjectionToken, Input, HostListener, HostBinding } from '@angular/core'; + import { Directive, Inject, InjectionToken, Input, HostListener, HostBinding, ViewContainerRef, TemplateRef } from '@angular/core'; const INJECTED_TOKEN = new InjectionToken('injected'); - const ViewContainerRef = {}; - const TemplateRef = {}; + const TestToken = {}; class SomeDirective { constructor(_viewContainer, _template, injected) { @@ -196,6 +195,7 @@ runInEachFileSystem(() => { class NoArgsProperty { } NoArgsProperty.decorators = [ + { type: Directive }, ]; @@ -1099,6 +1099,7 @@ runInEachFileSystem(() => { describe('getConstructorParameters()', () => { it('should find the decorated constructor parameters', () => { + loadFakeCore(getFileSystem()); loadTestFiles([SOME_DIRECTIVE_FILE]); const {program} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name); const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); @@ -1111,7 +1112,7 @@ runInEachFileSystem(() => { '_viewContainer', '_template', 'injected' ]); expectTypeValueReferencesForParameters( - parameters, ['ViewContainerRef', 'TemplateRef', null]); + parameters, ['ViewContainerRef', 'TemplateRef', null], '@angular/core'); }); it('should accept `ctorParameters` as an array', () => { @@ -1499,23 +1500,15 @@ runInEachFileSystem(() => { describe('getDeclarationOfIdentifier()', () => { it('should return the declaration of a locally defined identifier', () => { + loadFakeCore(getFileSystem()); loadTestFiles([SOME_DIRECTIVE_FILE]); const {program} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name); const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker()); const classNode = getDeclaration( program, SOME_DIRECTIVE_FILE.name, 'SomeDirective', isNamedClassDeclaration); - const ctrDecorators = host.getConstructorParameters(classNode) !; - const identifierOfViewContainerRef = (ctrDecorators[0].typeValueReference !as{ - local: true, - expression: ts.Identifier, - defaultImportStatement: null, - }).expression; - - const expectedDeclarationNode = getDeclaration( - program, SOME_DIRECTIVE_FILE.name, 'ViewContainerRef', isNamedVariableDeclaration); - const actualDeclaration = host.getDeclarationOfIdentifier(identifierOfViewContainerRef); + const actualDeclaration = host.getDeclarationOfIdentifier(classNode.name); expect(actualDeclaration).not.toBe(null); - expect(actualDeclaration !.node).toBe(expectedDeclarationNode); + expect(actualDeclaration !.node).toBe(classNode); expect(actualDeclaration !.viaModule).toBe(null); }); diff --git a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts index 4cdc43c50a..df14a01567 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -1560,9 +1560,6 @@ runInEachFileSystem(() => { expect(decorators.length).toEqual(1); expect(decorators[0].import).toBe(mockImportInfo); - - const typeIdentifier = spy.calls.mostRecent().args[0] as ts.Identifier; - expect(typeIdentifier.text).toBe('Inject'); }); }); diff --git a/packages/compiler-cli/ngcc/test/host/util.ts b/packages/compiler-cli/ngcc/test/host/util.ts index 09bb27282b..7e6a9de5ee 100644 --- a/packages/compiler-cli/ngcc/test/host/util.ts +++ b/packages/compiler-cli/ngcc/test/host/util.ts @@ -14,15 +14,26 @@ import {CtorParameter} from '../../../src/ngtsc/reflection'; * names. */ export function expectTypeValueReferencesForParameters( - parameters: CtorParameter[], expectedParams: (string | null)[]) { + parameters: CtorParameter[], expectedParams: (string | null)[], + fromModule: string | null = null) { parameters !.forEach((param, idx) => { const expected = expectedParams[idx]; if (expected !== null) { - if (param.typeValueReference === null || !param.typeValueReference.local || - !ts.isIdentifier(param.typeValueReference.expression)) { + if (param.typeValueReference === null) { fail(`Incorrect typeValueReference generated, expected ${expected}`); - } else { - expect(param.typeValueReference.expression.text).toEqual(expected); + } else if (param.typeValueReference.local && fromModule !== null) { + fail(`Incorrect typeValueReference generated, expected non-local`); + } else if (!param.typeValueReference.local && fromModule === null) { + fail(`Incorrect typeValueReference generated, expected local`); + } else if (param.typeValueReference.local) { + if (!ts.isIdentifier(param.typeValueReference.expression)) { + fail(`Incorrect typeValueReference generated, expected identifer`); + } else { + expect(param.typeValueReference.expression.text).toEqual(expected); + } + } else if (param.typeValueReference !== null) { + expect(param.typeValueReference.moduleName).toBe(fromModule !); + expect(param.typeValueReference.name).toBe(expected); } } });