honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
JoostK 2020-09-17 20:40:35 +02:00 committed by Misko Hevery
parent d795a00137
commit e4424863c2
7 changed files with 92 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
packages/compiler-cli/ngcc/src/host/esm2015_host.ts
View File

@ -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.

4
packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts
View File

@ -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', () => {

8
packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts
View File

@ -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', () => {

5
packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts
View File

@ -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', () => {

6
packages/compiler-cli/ngcc/test/host/umd_host_spec.ts
View File

@ -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', () => {

29
packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
View File

@ -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

13
packages/compiler-cli/ngcc/test/integration/util.ts
View File

@ -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: [],