From e4424863c2cebdf3786f82c336294195d7733c93 Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 17 Sep 2020 20:40:35 +0200 Subject: [PATCH] fix(ngcc): fix compilation of `ChangeDetectorRef` in pipe constructors (#38892) In #38666 we changed how ngcc deals with type expressions, where it would now always emit the original type expression into the generated code as a "local" type value reference instead of synthesizing new imports using an "imported" type value reference. This was done as a fix to properly deal with renamed symbols, however it turns out that the compiler has special handling for certain imported symbols, e.g. `ChangeDetectorRef` from `@angular/core`. The "local" type value reference prevented this special logic from being hit, resulting in incorrect compilation of pipe factories. This commit fixes the issue by manually inspecting the import of the type expression, in order to return an "imported" type value reference. By manually inspecting the import we continue to handle renamed symbols. Fixes #38883 PR Close #38892 --- .../ngcc/src/host/esm2015_host.ts | 46 +++++++++++++++---- .../ngcc/test/host/commonjs_host_spec.ts | 4 +- .../ngcc/test/host/esm2015_host_spec.ts | 8 ++-- .../ngcc/test/host/esm5_host_spec.ts | 5 +- .../ngcc/test/host/umd_host_spec.ts | 6 +-- .../ngcc/test/integration/ngcc_spec.ts | 29 ++++++++++++ .../ngcc/test/integration/util.ts | 13 +++++- 7 files changed, 92 insertions(+), 19 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index 343880920c4c9..3ce41f290e868 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -10,7 +10,7 @@ import * as ts from 'typescript'; import {absoluteFromSourceFile} from '../../../src/ngtsc/file_system'; import {Logger} from '../../../src/ngtsc/logging'; -import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, EnumMember, isDecoratorIdentifier, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, KnownDeclaration, reflectObjectLiteral, SpecialDeclarationKind, TypeScriptReflectionHost, TypeValueReference, TypeValueReferenceKind, ValueUnavailableKind} from '../../../src/ngtsc/reflection'; +import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, EnumMember, Import, isDecoratorIdentifier, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, KnownDeclaration, reflectObjectLiteral, SpecialDeclarationKind, TypeScriptReflectionHost, TypeValueReference, TypeValueReferenceKind, ValueUnavailableKind} from '../../../src/ngtsc/reflection'; import {isWithinPackage} from '../analysis/util'; import {BundleProgram} from '../packages/bundle_program'; import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils'; @@ -1608,10 +1608,11 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N /** * Compute the `TypeValueReference` for the given `typeExpression`. * - * In ngcc, all the `typeExpression` are guaranteed to be "values" because it is working in JS and - * not TS. This means that the TS compiler is not going to remove the "type" import and so we can - * always use a LOCAL `TypeValueReference` kind, rather than trying to force an additional import - * for non-local expressions. + * Although `typeExpression` is a valid `ts.Expression` that could be emitted directly into the + * generated code, ngcc still needs to resolve the declaration and create an `IMPORTED` type + * value reference as the compiler has specialized handling for some symbols, for example + * `ChangeDetectorRef` from `@angular/core`. Such an `IMPORTED` type value reference will result + * in a newly generated namespace import, instead of emitting the original `typeExpression` as is. */ private typeToValue(typeExpression: ts.Expression|null): TypeValueReference { if (typeExpression === null) { @@ -1621,13 +1622,42 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N }; } + const imp = this.getImportOfExpression(typeExpression); + const decl = this.getDeclarationOfExpression(typeExpression); + if (imp === null || decl === null || decl.node === null) { + return { + kind: TypeValueReferenceKind.LOCAL, + expression: typeExpression, + defaultImportStatement: null, + }; + } + return { - kind: TypeValueReferenceKind.LOCAL, - expression: typeExpression, - defaultImportStatement: null, + kind: TypeValueReferenceKind.IMPORTED, + valueDeclaration: decl.node, + moduleName: imp.from, + importedName: imp.name, + nestedPath: null, }; } + /** + * Determines where the `expression` is imported from. + * + * @param expression the expression to determine the import details for. + * @returns the `Import` for the expression, or `null` if the expression is not imported or the + * expression syntax is not supported. + */ + private getImportOfExpression(expression: ts.Expression): Import|null { + if (ts.isIdentifier(expression)) { + return this.getImportOfIdentifier(expression); + } else if (ts.isPropertyAccessExpression(expression) && ts.isIdentifier(expression.name)) { + return this.getImportOfIdentifier(expression.name); + } else { + return null; + } + } + /** * Get the parameter type and decorators for the constructor of a class, * where the information is stored on a static property of the class. diff --git a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts index c7d0f243b0c3e..366c15ff6113a 100644 --- a/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts @@ -1211,7 +1211,7 @@ exports.MissingClass2 = MissingClass2; }); describe('getConstructorParameters', () => { - it('should always specify LOCAL type value references for decorated constructor parameter types', + it('should retain imported name for type value references for decorated constructor parameter types', () => { const files = [ { @@ -1271,7 +1271,7 @@ exports.MissingClass2 = MissingClass2; expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); expectTypeValueReferencesForParameters( - parameters, ['shared.Baz', 'local.External', 'SameFile']); + parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]); }); it('should find the decorated constructor parameters', () => { 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 96182d3e4858c..a0be48dc70935 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts @@ -1140,7 +1140,7 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters()', () => { - it('should always specify LOCAL type value references for decorated constructor parameter types', + it('should retain imported name for type value references for decorated constructor parameter types', () => { const files = [ { @@ -1188,7 +1188,8 @@ runInEachFileSystem(() => { const parameters = host.getConstructorParameters(classNode)!; expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); - expectTypeValueReferencesForParameters(parameters, ['Baz', 'External', 'SameFile']); + expectTypeValueReferencesForParameters( + parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]); }); it('should find the decorated constructor parameters', () => { @@ -1205,7 +1206,8 @@ runInEachFileSystem(() => { '_viewContainer', '_template', 'injected' ]); expectTypeValueReferencesForParameters( - parameters, ['ViewContainerRef', 'TemplateRef', null]); + parameters, ['ViewContainerRef', 'TemplateRef', null], + ['@angular/core', '@angular/core', null]); }); it('should accept `ctorParameters` as an array', () => { diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts index dafb07947d6a3..e5e3f0c75de0c 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts @@ -1252,7 +1252,7 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters()', () => { - it('should always specify LOCAL type value references for decorated constructor parameter types', + it('should retain imported name for type value references for decorated constructor parameter types', () => { const files = [ { @@ -1310,7 +1310,8 @@ runInEachFileSystem(() => { const parameters = host.getConstructorParameters(classNode)!; expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); - expectTypeValueReferencesForParameters(parameters, ['Baz', 'External', 'SameFile']); + expectTypeValueReferencesForParameters( + parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]); }); it('should find the decorated constructor parameters', () => { 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 cf918fb43f533..25a40b8626712 100644 --- a/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/umd_host_spec.ts @@ -1332,7 +1332,7 @@ runInEachFileSystem(() => { }); describe('getConstructorParameters', () => { - it('should always specify LOCAL type value references for decorated constructor parameter types', + it('should retain imported name for type value references for decorated constructor parameter types', () => { const files = [ { @@ -1369,7 +1369,7 @@ runInEachFileSystem(() => { name: _('/main.js'), contents: ` (function (global, factory) { - typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib), require('./local')) : + typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib'), require('./local')) : typeof define === 'function' && define.amd ? define('main', ['exports', 'shared-lib', './local'], factory) : (factory(global.main, global.shared, global.local)); }(this, (function (exports, shared, local) { 'use strict'; @@ -1401,7 +1401,7 @@ runInEachFileSystem(() => { expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']); expectTypeValueReferencesForParameters( - parameters, ['shared.Baz', 'local.External', 'SameFile']); + parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]); }); it('should find the decorated constructor parameters', () => { diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 16c7dc89351ab..7208abe4df885 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -576,6 +576,35 @@ runInEachFileSystem(() => { `TestClass.ɵprov = ɵngcc0.ɵɵdefineInjectable({`); }); + // https://github.com/angular/angular/issues/38883 + it('should recognize ChangeDetectorRef as special symbol for pipes', () => { + compileIntoFlatEs2015Package('test-package', { + '/index.ts': ` + import {ChangeDetectorRef, Pipe, PipeTransform} from '@angular/core'; + + @Pipe({ + name: 'myTestPipe' + }) + export class TestClass implements PipeTransform { + constructor(cdr: ChangeDetectorRef) {} + transform(value: any) { return value; } + } + `, + }); + + mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: 'test-package', + propertiesToConsider: ['esm2015'], + }); + + const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`)); + expect(jsContents) + .toContain( + `TestClass.ɵfac = function TestClass_Factory(t) { ` + + `return new (t || TestClass)(ɵngcc0.ɵɵinjectPipeChangeDetectorRef()); };`); + }); + it('should use the correct type name in typings files when an export has a different name in source files', () => { // We need to make sure that changes to the typings files use the correct name diff --git a/packages/compiler-cli/ngcc/test/integration/util.ts b/packages/compiler-cli/ngcc/test/integration/util.ts index 6f0439e69b80b..e8ab286ee2422 100644 --- a/packages/compiler-cli/ngcc/test/integration/util.ts +++ b/packages/compiler-cli/ngcc/test/integration/util.ts @@ -99,7 +99,14 @@ function compileIntoFlatPackage( program.emit(); }; - emit({declaration: true, module: options.module, target: options.target, lib: []}); + emit({ + declaration: true, + emitDecoratorMetadata: true, + moduleResolution: ts.ModuleResolutionKind.NodeJs, + module: options.module, + target: options.target, + lib: [], + }); // Copy over the JS and .d.ts files, and add a .metadata.json for each .d.ts file. for (const file of rootNames) { @@ -152,7 +159,9 @@ export function compileIntoApf( compileFs.ensureDir(compileFs.resolve('esm2015')); emit({ declaration: true, + emitDecoratorMetadata: true, outDir: './esm2015', + moduleResolution: ts.ModuleResolutionKind.NodeJs, module: ts.ModuleKind.ESNext, target: ts.ScriptTarget.ES2015, lib: [], @@ -178,7 +187,9 @@ export function compileIntoApf( compileFs.ensureDir(compileFs.resolve('esm5')); emit({ declaration: false, + emitDecoratorMetadata: true, outDir: './esm5', + moduleResolution: ts.ModuleResolutionKind.NodeJs, module: ts.ModuleKind.ESNext, target: ts.ScriptTarget.ES5, lib: [],