Skip to content

Commit c124286

Browse files
ahnpnlahnpnl
authored
refactor(transformers): update downlevel ctor transformer (#730)
Follow the fix angular/angular#40374
1 parent f943ef6 commit c124286

File tree

2 files changed

+152
-127
lines changed

2 files changed

+152
-127
lines changed

‎src/transformers/downlevel-ctor.ts‎

Lines changed: 12 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -11,107 +11,7 @@
1111
import { Decorator, ReflectionHost, TypeScriptReflectionHost } from '@angular/compiler-cli/src/ngtsc/reflection';
1212
import ts from 'typescript';
1313

14-
/**
15-
* Describes a TypeScript transformation context with the internal emit
16-
* resolver exposed. There are requests upstream in TypeScript to expose
17-
* that as public API: https://github.com/microsoft/TypeScript/issues/17516..
18-
*/
19-
interface TransformationContextWithResolver extends ts.TransformationContext {
20-
getEmitResolver: () => EmitResolver;
21-
}
22-
23-
/** Describes a subset of the TypeScript internal emit resolver. */
24-
interface EmitResolver {
25-
isReferencedAliasDeclaration?(node: ts.Node, checkChildren?: boolean): void;
26-
}
27-
28-
/**
29-
* Patches the alias declaration reference resolution for a given transformation context
30-
* so that TypeScript knows about the specified alias declarations being referenced.
31-
*
32-
* This exists because TypeScript performs analysis of import usage before transformers
33-
* run and doesn't refresh its state after transformations. This means that imports
34-
* for symbols used as constructor types are elided due to their original type-only usage.
35-
*
36-
* In reality though, since we downlevel decorators and constructor parameters, we want
37-
* these symbols to be retained in the JavaScript output as they will be used as values
38-
* at runtime. We can instruct TypeScript to preserve imports for such identifiers by
39-
* creating a mutable clone of a given import specifier/clause or namespace, but that
40-
* has the downside of preserving the full import in the JS output. See:
41-
* https://github.com/microsoft/TypeScript/blob/3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/src/compiler/transformers/ts.ts#L242-L250.
42-
*
43-
* This is a trick the CLI used in the past for constructor parameter downleveling in JIT:
44-
* https://github.com/angular/angular-cli/blob/b3f84cc5184337666ce61c07b7b9df418030106f/packages/ngtools/webpack/src/transformers/ctor-parameters.ts#L323-L325
45-
* The trick is not ideal though as it preserves the full import (as outlined before), and it
46-
* results in a slow-down due to the type checker being involved multiple times. The CLI
47-
* worked around this import preserving issue by having another complex post-process step that
48-
* detects and elides unused imports. Note that these unused imports could cause unused chunks
49-
* being generated by Webpack if the application or library is not marked as side-effect free.
50-
*
51-
* This is not ideal though, as we basically re-implement the complex import usage resolution
52-
* from TypeScript. We can do better by letting TypeScript do the import eliding, but providing
53-
* information about the alias declarations (e.g. import specifiers) that should not be elided
54-
* because they are actually referenced (as they will now appear in static properties).
55-
*
56-
* More information about these limitations with transformers can be found in:
57-
* 1. https://github.com/Microsoft/TypeScript/issues/17552.
58-
* 2. https://github.com/microsoft/TypeScript/issues/17516.
59-
* 3. https://github.com/angular/tsickle/issues/635.
60-
*
61-
* The patch we apply to tell TypeScript about actual referenced aliases (i.e. imported symbols),
62-
* matches conceptually with the logic that runs internally in TypeScript when the
63-
* `emitDecoratorMetadata` flag is enabled. TypeScript basically surfaces the same problem and
64-
* solves it conceptually the same way, but obviously doesn't need to access an `@internal` API.
65-
*
66-
* See below. Note that this uses sourcegraph as the TypeScript checker file doesn't display on
67-
* Github.
68-
* https://sourcegraph.com/github.com/microsoft/TypeScript@3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/-/blob/src/compiler/checker.ts#L31219-31257
69-
*/
70-
function patchAliasReferenceResolutionOrDie(
71-
context: ts.TransformationContext,
72-
referencedAliases: Set<ts.Declaration>,
73-
): void {
74-
// If the `getEmitResolver` method is not available, TS most likely changed the
75-
// internal structure of the transformation context. We will abort gracefully.
76-
if (!isTransformationContextWithEmitResolver(context)) {
77-
throwIncompatibleTransformationContextError();
78-
79-
return;
80-
}
81-
const emitResolver = context.getEmitResolver();
82-
// eslint-disable-next-line @typescript-eslint/unbound-method
83-
const originalReferenceResolution = emitResolver.isReferencedAliasDeclaration;
84-
// If the emit resolver does not have a function called `isReferencedAliasDeclaration`, then
85-
// we abort gracefully as most likely TS changed the internal structure of the emit resolver.
86-
if (originalReferenceResolution === undefined) {
87-
throwIncompatibleTransformationContextError();
88-
89-
return;
90-
}
91-
emitResolver.isReferencedAliasDeclaration = function (node, ...args) {
92-
if (isAliasImportDeclaration(node) && referencedAliases.has(node)) {
93-
return true;
94-
}
95-
96-
return originalReferenceResolution.call(emitResolver, node, ...args);
97-
};
98-
}
99-
100-
/** Whether the transformation context exposes its emit resolver. */
101-
function isTransformationContextWithEmitResolver(
102-
context: ts.TransformationContext,
103-
): context is TransformationContextWithResolver {
104-
return (context as Partial<TransformationContextWithResolver>).getEmitResolver !== undefined;
105-
}
106-
107-
/**
108-
* Gets whether a given node corresponds to an import alias declaration. Alias
109-
* declarations can be import specifiers, namespace imports or import clauses
110-
* as these do not declare an actual symbol but just point to a target declaration.
111-
*/
112-
function isAliasImportDeclaration(node: ts.Node): node is ts.ImportSpecifier | ts.NamespaceImport | ts.ImportClause {
113-
return ts.isImportSpecifier(node) || ts.isNamespaceImport(node) || ts.isImportClause(node);
114-
}
14+
import { isAliasImportDeclaration, loadIsReferencedAliasDeclarationPatch } from './patch-alias-reference-resolution';
11515

11616
/**
11717
* Whether a given decorator should be treated as an Angular decorator.
@@ -532,7 +432,12 @@ function getDownlevelDecoratorsTransform(
532432
skipClassDecorators: boolean,
533433
): ts.TransformerFactory<ts.SourceFile> {
534434
return (context: ts.TransformationContext) => {
535-
const referencedParameterTypes = new Set<ts.Declaration>();
435+
// Ensure that referenced type symbols are not elided by TypeScript. Imports for
436+
// such parameter type symbols previously could be type-only, but now might be also
437+
// used in the `ctorParameters` static property as a value. We want to make sure
438+
// that TypeScript does not elide imports for such type references. Read more
439+
// about this in the description for `loadIsReferencedAliasDeclarationPatch`.
440+
const referencedParameterTypes = loadIsReferencedAliasDeclarationPatch(context);
536441

537442
/**
538443
* Converts an EntityName (from a type annotation) to an expression (accessing a value).
@@ -627,7 +532,7 @@ function getDownlevelDecoratorsTransform(
627532
return [undefined, element, []];
628533
}
629534

630-
const name = element.name.text;
535+
const name = (element.name as ts.Identifier).text;
631536
const mutable = ts.getMutableClone(element);
632537
// eslint-disable-next-line @typescript-eslint/no-explicit-any
633538
(mutable as any).decorators = decoratorsToKeep.length
@@ -663,14 +568,16 @@ function getDownlevelDecoratorsTransform(
663568
decoratorsToKeep.push(decoratorNode);
664569
continue;
665570
}
666-
paramInfo.decorators.push(decoratorNode);
571+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
572+
paramInfo!.decorators.push(decoratorNode);
667573
}
668574
if (param.type) {
669575
// param has a type provided, e.g. "foo: Bar".
670576
// The type will be emitted as a value expression in entityNameToExpression, which takes
671577
// care not to emit anything for types that cannot be expressed as a value (e.g.
672578
// interfaces).
673-
paramInfo.type = param.type;
579+
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
580+
paramInfo!.type = param.type;
674581
}
675582
parametersInfo.push(paramInfo);
676583
const newParam = ts.updateParameter(
@@ -815,13 +722,6 @@ function getDownlevelDecoratorsTransform(
815722
}
816723

817724
return (sf: ts.SourceFile) => {
818-
// Ensure that referenced type symbols are not elided by TypeScript. Imports for
819-
// such parameter type symbols previously could be type-only, but now might be also
820-
// used in the `ctorParameters` static property as a value. We want to make sure
821-
// that TypeScript does not elide imports for such type references. Read more
822-
// about this in the description for `patchAliasReferenceResolution`.
823-
patchAliasReferenceResolutionOrDie(context, referencedParameterTypes);
824-
825725
// Downlevel decorators and constructor parameter types. We will keep track of all
826726
// referenced constructor parameter types so that we can instruct TypeScript to
827727
// not elide their imports if they previously were only type-only.
@@ -830,21 +730,6 @@ function getDownlevelDecoratorsTransform(
830730
};
831731
}
832732

833-
/**
834-
* Throws an error about an incompatible TypeScript version for which the alias
835-
* declaration reference resolution could not be monkey-patched. The error will
836-
* also propose potential solutions that can be applied by developers.
837-
*/
838-
function throwIncompatibleTransformationContextError() {
839-
throw Error(
840-
'Unable to downlevel Angular decorators due to an incompatible TypeScript ' +
841-
'version.\nIf you recently updated TypeScript and this issue surfaces now, consider ' +
842-
'downgrading.\n\n' +
843-
'Please report an issue on the Angular repositories when this issue ' +
844-
'surfaces and you are using a supposedly compatible TypeScript version.',
845-
);
846-
}
847-
848733
/**
849734
* Transform for downleveling Angular decorators and Angular-decorated class constructor
850735
* parameters for dependency injection. This transform can be used by the CLI for JIT-mode

‎src/transformers/patch-alias-reference-resolution.ts‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import ts from 'typescript';
10+
11+
/**
12+
* Describes a TypeScript transformation context with the internal emit
13+
* resolver exposed. There are requests upstream in TypeScript to expose
14+
* that as public API: https://github.com/microsoft/TypeScript/issues/17516..
15+
*/
16+
interface TransformationContextWithResolver extends ts.TransformationContext {
17+
getEmitResolver: () => EmitResolver;
18+
}
19+
20+
const patchedReferencedAliasesSymbol = Symbol('patchedReferencedAliases');
21+
22+
/** Describes a subset of the TypeScript internal emit resolver. */
23+
interface EmitResolver {
24+
isReferencedAliasDeclaration?(node: ts.Node, ...args: unknown[]): void;
25+
[patchedReferencedAliasesSymbol]?: Set<ts.Declaration>;
26+
}
27+
28+
/**
29+
* Patches the alias declaration reference resolution for a given transformation context
30+
* so that TypeScript knows about the specified alias declarations being referenced.
31+
*
32+
* This exists because TypeScript performs analysis of import usage before transformers
33+
* run and doesn't refresh its state after transformations. This means that imports
34+
* for symbols used as constructor types are elided due to their original type-only usage.
35+
*
36+
* In reality though, since we downlevel decorators and constructor parameters, we want
37+
* these symbols to be retained in the JavaScript output as they will be used as values
38+
* at runtime. We can instruct TypeScript to preserve imports for such identifiers by
39+
* creating a mutable clone of a given import specifier/clause or namespace, but that
40+
* has the downside of preserving the full import in the JS output. See:
41+
* https://github.com/microsoft/TypeScript/blob/3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/src/compiler/transformers/ts.ts#L242-L250.
42+
*
43+
* This is a trick the CLI used in the past for constructor parameter downleveling in JIT:
44+
* https://github.com/angular/angular-cli/blob/b3f84cc5184337666ce61c07b7b9df418030106f/packages/ngtools/webpack/src/transformers/ctor-parameters.ts#L323-L325
45+
* The trick is not ideal though as it preserves the full import (as outlined before), and it
46+
* results in a slow-down due to the type checker being involved multiple times. The CLI worked
47+
* around this import preserving issue by having another complex post-process step that detects and
48+
* elides unused imports. Note that these unused imports could cause unused chunks being generated
49+
* by Webpack if the application or library is not marked as side-effect free.
50+
*
51+
* This is not ideal though, as we basically re-implement the complex import usage resolution
52+
* from TypeScript. We can do better by letting TypeScript do the import eliding, but providing
53+
* information about the alias declarations (e.g. import specifiers) that should not be elided
54+
* because they are actually referenced (as they will now appear in static properties).
55+
*
56+
* More information about these limitations with transformers can be found in:
57+
* 1. https://github.com/Microsoft/TypeScript/issues/17552.
58+
* 2. https://github.com/microsoft/TypeScript/issues/17516.
59+
* 3. https://github.com/angular/tsickle/issues/635.
60+
*
61+
* The patch we apply to tell TypeScript about actual referenced aliases (i.e. imported symbols),
62+
* matches conceptually with the logic that runs internally in TypeScript when the
63+
* `emitDecoratorMetadata` flag is enabled. TypeScript basically surfaces the same problem and
64+
* solves it conceptually the same way, but obviously doesn't need to access an `@internal` API.
65+
*
66+
* The set that is returned by this function is meant to be filled with import declaration nodes
67+
* that have been referenced in a value-position by the transform, such the the installed patch can
68+
* ensure that those import declarations are not elided.
69+
*
70+
* See below. Note that this uses sourcegraph as the TypeScript checker file doesn't display on
71+
* Github.
72+
* https://sourcegraph.com/github.com/microsoft/TypeScript@3eaa7c65f6f076a08a5f7f1946fd0df7c7430259/-/blob/src/compiler/checker.ts#L31219-31257
73+
*/
74+
export function loadIsReferencedAliasDeclarationPatch(context: ts.TransformationContext): Set<ts.Declaration> {
75+
// If the `getEmitResolver` method is not available, TS most likely changed the
76+
// internal structure of the transformation context. We will abort gracefully.
77+
if (!isTransformationContextWithEmitResolver(context)) {
78+
throwIncompatibleTransformationContextError();
79+
}
80+
const emitResolver = context.getEmitResolver();
81+
82+
// The emit resolver may have been patched already, in which case we return the set of referenced
83+
// aliases that was created when the patch was first applied.
84+
// See https://github.com/angular/angular/issues/40276.
85+
const existingReferencedAliases = emitResolver[patchedReferencedAliasesSymbol];
86+
if (existingReferencedAliases !== undefined) {
87+
return existingReferencedAliases;
88+
}
89+
90+
const originalIsReferencedAliasDeclaration = emitResolver.isReferencedAliasDeclaration;
91+
// If the emit resolver does not have a function called `isReferencedAliasDeclaration`, then
92+
// we abort gracefully as most likely TS changed the internal structure of the emit resolver.
93+
if (originalIsReferencedAliasDeclaration === undefined) {
94+
throwIncompatibleTransformationContextError();
95+
}
96+
97+
const referencedAliases = new Set<ts.Declaration>();
98+
emitResolver.isReferencedAliasDeclaration = function (node, ...args) {
99+
if (isAliasImportDeclaration(node) && referencedAliases.has(node)) {
100+
return true;
101+
}
102+
103+
return originalIsReferencedAliasDeclaration.call(emitResolver, node, ...args);
104+
};
105+
106+
return (emitResolver[patchedReferencedAliasesSymbol] = referencedAliases);
107+
}
108+
109+
/**
110+
* Gets whether a given node corresponds to an import alias declaration. Alias
111+
* declarations can be import specifiers, namespace imports or import clauses
112+
* as these do not declare an actual symbol but just point to a target declaration.
113+
*/
114+
export function isAliasImportDeclaration(
115+
node: ts.Node,
116+
): node is ts.ImportSpecifier | ts.NamespaceImport | ts.ImportClause {
117+
return ts.isImportSpecifier(node) || ts.isNamespaceImport(node) || ts.isImportClause(node);
118+
}
119+
120+
/** Whether the transformation context exposes its emit resolver. */
121+
function isTransformationContextWithEmitResolver(
122+
context: ts.TransformationContext,
123+
): context is TransformationContextWithResolver {
124+
return (context as Partial<TransformationContextWithResolver>).getEmitResolver !== undefined;
125+
}
126+
127+
/**
128+
* Throws an error about an incompatible TypeScript version for which the alias
129+
* declaration reference resolution could not be monkey-patched. The error will
130+
* also propose potential solutions that can be applied by developers.
131+
*/
132+
function throwIncompatibleTransformationContextError(): never {
133+
throw Error(
134+
'Unable to downlevel Angular decorators due to an incompatible TypeScript ' +
135+
'version.\nIf you recently updated TypeScript and this issue surfaces now, consider ' +
136+
'downgrading.\n\n' +
137+
'Please report an issue on the Angular repositories when this issue ' +
138+
'surfaces and you are using a supposedly compatible TypeScript version.',
139+
);
140+
}

0 commit comments

Comments
 (0)