Sitelet https://github.com/angular/angular/commit/093c3a1
Skip to content

Commit 093c3a1

Browse files
JoostKmhevery
JoostK
authored and
mhevery
committed
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
1 parent a0756e9 commit 093c3a1

7 files changed

Lines changed: 92 additions & 19 deletions

File tree

‎packages/compiler-cli/ngcc/src/host/esm2015_host.ts‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import * as ts from 'typescript';
1010

1111
import {absoluteFromSourceFile} from '../../../src/ngtsc/file_system';
1212
import {Logger} from '../../../src/ngtsc/logging';
13-
import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, EnumMember, isDecoratorIdentifier, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, KnownDeclaration, reflectObjectLiteral, SpecialDeclarationKind, TypeScriptReflectionHost, TypeValueReference, TypeValueReferenceKind, ValueUnavailableKind} from '../../../src/ngtsc/reflection';
13+
import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, EnumMember, Import, isDecoratorIdentifier, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, KnownDeclaration, reflectObjectLiteral, SpecialDeclarationKind, TypeScriptReflectionHost, TypeValueReference, TypeValueReferenceKind, ValueUnavailableKind} from '../../../src/ngtsc/reflection';
1414
import {isWithinPackage} from '../analysis/util';
1515
import {BundleProgram} from '../packages/bundle_program';
1616
import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils';
@@ -1608,10 +1608,11 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
16081608
/**
16091609
* Compute the `TypeValueReference` for the given `typeExpression`.
16101610
*
1611-
* In ngcc, all the `typeExpression` are guaranteed to be "values" because it is working in JS and
1612-
* not TS. This means that the TS compiler is not going to remove the "type" import and so we can
1613-
* always use a LOCAL `TypeValueReference` kind, rather than trying to force an additional import
1614-
* for non-local expressions.
1611+
* Although `typeExpression` is a valid `ts.Expression` that could be emitted directly into the
1612+
* generated code, ngcc still needs to resolve the declaration and create an `IMPORTED` type
1613+
* value reference as the compiler has specialized handling for some symbols, for example
1614+
* `ChangeDetectorRef` from `@angular/core`. Such an `IMPORTED` type value reference will result
1615+
* in a newly generated namespace import, instead of emitting the original `typeExpression` as is.
16151616
*/
16161617
private typeToValue(typeExpression: ts.Expression|null): TypeValueReference {
16171618
if (typeExpression === null) {
@@ -1621,13 +1622,42 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
16211622
};
16221623
}
16231624

1625+
const imp = this.getImportOfExpression(typeExpression);
1626+
const decl = this.getDeclarationOfExpression(typeExpression);
1627+
if (imp === null || decl === null || decl.node === null) {
1628+
return {
1629+
kind: TypeValueReferenceKind.LOCAL,
1630+
expression: typeExpression,
1631+
defaultImportStatement: null,
1632+
};
1633+
}
1634+
16241635
return {
1625-
kind: TypeValueReferenceKind.LOCAL,
1626-
expression: typeExpression,
1627-
defaultImportStatement: null,
1636+
kind: TypeValueReferenceKind.IMPORTED,
1637+
valueDeclaration: decl.node,
1638+
moduleName: imp.from,
1639+
importedName: imp.name,
1640+
nestedPath: null,
16281641
};
16291642
}
16301643

1644+
/**
1645+
* Determines where the `expression` is imported from.
1646+
*
1647+
* @param expression the expression to determine the import details for.
1648+
* @returns the `Import` for the expression, or `null` if the expression is not imported or the
1649+
* expression syntax is not supported.
1650+
*/
1651+
private getImportOfExpression(expression: ts.Expression): Import|null {
1652+
if (ts.isIdentifier(expression)) {
1653+
return this.getImportOfIdentifier(expression);
1654+
} else if (ts.isPropertyAccessExpression(expression) && ts.isIdentifier(expression.name)) {
1655+
return this.getImportOfIdentifier(expression.name);
1656+
} else {
1657+
return null;
1658+
}
1659+
}
1660+
16311661
/**
16321662
* Get the parameter type and decorators for the constructor of a class,
16331663
* where the information is stored on a static property of the class.

‎packages/compiler-cli/ngcc/test/host/commonjs_host_spec.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,7 +1211,7 @@ exports.MissingClass2 = MissingClass2;
12111211
});
12121212

12131213
describe('getConstructorParameters', () => {
1214-
it('should always specify LOCAL type value references for decorated constructor parameter types',
1214+
it('should retain imported name for type value references for decorated constructor parameter types',
12151215
() => {
12161216
const files = [
12171217
{
@@ -1271,7 +1271,7 @@ exports.MissingClass2 = MissingClass2;
12711271

12721272
expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']);
12731273
expectTypeValueReferencesForParameters(
1274-
parameters, ['shared.Baz', 'local.External', 'SameFile']);
1274+
parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]);
12751275
});
12761276

12771277
it('should find the decorated constructor parameters', () => {

‎packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1140,7 +1140,7 @@ runInEachFileSystem(() => {
11401140
});
11411141

11421142
describe('getConstructorParameters()', () => {
1143-
it('should always specify LOCAL type value references for decorated constructor parameter types',
1143+
it('should retain imported name for type value references for decorated constructor parameter types',
11441144
() => {
11451145
const files = [
11461146
{
@@ -1188,7 +1188,8 @@ runInEachFileSystem(() => {
11881188
const parameters = host.getConstructorParameters(classNode)!;
11891189

11901190
expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']);
1191-
expectTypeValueReferencesForParameters(parameters, ['Baz', 'External', 'SameFile']);
1191+
expectTypeValueReferencesForParameters(
1192+
parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]);
11921193
});
11931194

11941195
it('should find the decorated constructor parameters', () => {
@@ -1205,7 +1206,8 @@ runInEachFileSystem(() => {
12051206
'_viewContainer', '_template', 'injected'
12061207
]);
12071208
expectTypeValueReferencesForParameters(
1208-
parameters, ['ViewContainerRef', 'TemplateRef', null]);
1209+
parameters, ['ViewContainerRef', 'TemplateRef', null],
1210+
['@angular/core', '@angular/core', null]);
12091211
});
12101212

12111213
it('should accept `ctorParameters` as an array', () => {

‎packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1252,7 +1252,7 @@ runInEachFileSystem(() => {
12521252
});
12531253

12541254
describe('getConstructorParameters()', () => {
1255-
it('should always specify LOCAL type value references for decorated constructor parameter types',
1255+
it('should retain imported name for type value references for decorated constructor parameter types',
12561256
() => {
12571257
const files = [
12581258
{
@@ -1310,7 +1310,8 @@ runInEachFileSystem(() => {
13101310
const parameters = host.getConstructorParameters(classNode)!;
13111311

13121312
expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']);
1313-
expectTypeValueReferencesForParameters(parameters, ['Baz', 'External', 'SameFile']);
1313+
expectTypeValueReferencesForParameters(
1314+
parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]);
13141315
});
13151316

13161317
it('should find the decorated constructor parameters', () => {

‎packages/compiler-cli/ngcc/test/host/umd_host_spec.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1332,7 +1332,7 @@ runInEachFileSystem(() => {
13321332
});
13331333

13341334
describe('getConstructorParameters', () => {
1335-
it('should always specify LOCAL type value references for decorated constructor parameter types',
1335+
it('should retain imported name for type value references for decorated constructor parameter types',
13361336
() => {
13371337
const files = [
13381338
{
@@ -1369,7 +1369,7 @@ runInEachFileSystem(() => {
13691369
name: _('/main.js'),
13701370
contents: `
13711371
(function (global, factory) {
1372-
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib), require('./local')) :
1372+
typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('shared-lib'), require('./local')) :
13731373
typeof define === 'function' && define.amd ? define('main', ['exports', 'shared-lib', './local'], factory) :
13741374
(factory(global.main, global.shared, global.local));
13751375
}(this, (function (exports, shared, local) { 'use strict';
@@ -1401,7 +1401,7 @@ runInEachFileSystem(() => {
14011401

14021402
expect(parameters.map(p => p.name)).toEqual(['arg1', 'arg2', 'arg3']);
14031403
expectTypeValueReferencesForParameters(
1404-
parameters, ['shared.Baz', 'local.External', 'SameFile']);
1404+
parameters, ['Baz', 'External', 'SameFile'], ['shared-lib', './local', null]);
14051405
});
14061406

14071407
it('should find the decorated constructor parameters', () => {

‎packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,35 @@ runInEachFileSystem(() => {
576576
`TestClass.ɵprov = ɵngcc0.ɵɵdefineInjectable({`);
577577
});
578578

579+
// https://github.com/angular/angular/issues/38883
580+
it('should recognize ChangeDetectorRef as special symbol for pipes', () => {
581+
compileIntoFlatEs2015Package('test-package', {
582+
'/index.ts': `
583+
import {ChangeDetectorRef, Pipe, PipeTransform} from '@angular/core';
584+
585+
@Pipe({
586+
name: 'myTestPipe'
587+
})
588+
export class TestClass implements PipeTransform {
589+
constructor(cdr: ChangeDetectorRef) {}
590+
transform(value: any) { return value; }
591+
}
592+
`,
593+
});
594+
595+
mainNgcc({
596+
basePath: '/node_modules',
597+
targetEntryPointPath: 'test-package',
598+
propertiesToConsider: ['esm2015'],
599+
});
600+
601+
const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`));
602+
expect(jsContents)
603+
.toContain(
604+
`TestClass.ɵfac = function TestClass_Factory(t) { ` +
605+
`return new (t || TestClass)(ɵngcc0.ɵɵinjectPipeChangeDetectorRef()); };`);
606+
});
607+
579608
it('should use the correct type name in typings files when an export has a different name in source files',
580609
() => {
581610
// We need to make sure that changes to the typings files use the correct name

‎packages/compiler-cli/ngcc/test/integration/util.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,14 @@ function compileIntoFlatPackage(
9999
program.emit();
100100
};
101101

102-
emit({declaration: true, module: options.module, target: options.target, lib: []});
102+
emit({
103+
declaration: true,
104+
emitDecoratorMetadata: true,
105+
moduleResolution: ts.ModuleResolutionKind.NodeJs,
106+
module: options.module,
107+
target: options.target,
108+
lib: [],
109+
});
103110

104111
// Copy over the JS and .d.ts files, and add a .metadata.json for each .d.ts file.
105112
for (const file of rootNames) {
@@ -152,7 +159,9 @@ export function compileIntoApf(
152159
compileFs.ensureDir(compileFs.resolve('esm2015'));
153160
emit({
154161
declaration: true,
162+
emitDecoratorMetadata: true,
155163
outDir: './esm2015',
164+
moduleResolution: ts.ModuleResolutionKind.NodeJs,
156165
module: ts.ModuleKind.ESNext,
157166
target: ts.ScriptTarget.ES2015,
158167
lib: [],
@@ -178,7 +187,9 @@ export function compileIntoApf(
178187
compileFs.ensureDir(compileFs.resolve('esm5'));
179188
emit({
180189
declaration: false,
190+
emitDecoratorMetadata: true,
181191
outDir: './esm5',
192+
moduleResolution: ts.ModuleResolutionKind.NodeJs,
182193
module: ts.ModuleKind.ESNext,
183194
target: ts.ScriptTarget.ES5,
184195
lib: [],

0 commit comments

Comments
 (0)