Skip to content

Commit 03a2acd

Browse files
cexbrayatAndrewKushnir
cexbrayat
authored and
AndrewKushnir
committed
fix(core): properly remove imports in the afterRender phase migration (#56524)
Before this commit, the migration was removing the `AfterRenderPhase` enum from the imports but not the comma, resulting in invalid code: ts ``` import { , Directive, afterRender } from '@angular/core'; ``` This commit fixes this by using `updateNamedImports` and `replaceNode` instead of `removeNode`. After: ts ``` import { Directive, afterRender } from '@angular/core'; ``` PR Close #56524
1 parent 39df5fa commit 03a2acd

2 files changed

Lines changed: 65 additions & 47 deletions

File tree

‎packages/core/schematics/migrations/after-render-phase/migration.ts‎

Lines changed: 58 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@
88

99
import ts from 'typescript';
1010
import {ChangeTracker} from '../../utils/change_tracker';
11-
import {getImportOfIdentifier, getImportSpecifier} from '../../utils/typescript/imports';
11+
import {
12+
getImportOfIdentifier,
13+
getImportSpecifier,
14+
getNamedImports,
15+
} from '../../utils/typescript/imports';
1216

1317
const CORE = '@angular/core';
1418
const AFTER_RENDER_PHASE_ENUM = 'AfterRenderPhase';
@@ -22,57 +26,65 @@ export function migrateFile(
2226
rewriteFn: RewriteFn,
2327
) {
2428
const changeTracker = new ChangeTracker(ts.createPrinter());
29+
// Check if there are any imports of the `AfterRenderPhase` enum.
30+
const coreImports = getNamedImports(sourceFile, CORE);
31+
if (!coreImports) {
32+
return;
33+
}
2534
const phaseEnum = getImportSpecifier(sourceFile, CORE, AFTER_RENDER_PHASE_ENUM);
35+
if (!phaseEnum) {
36+
return;
37+
}
2638

27-
// Check if there are any imports of the `AfterRenderPhase` enum.
28-
if (phaseEnum) {
29-
// Remove the `AfterRenderPhase` enum import.
30-
changeTracker.removeNode(phaseEnum);
31-
ts.forEachChild(sourceFile, function visit(node: ts.Node) {
32-
ts.forEachChild(node, visit);
39+
// Remove the `AfterRenderPhase` enum import.
40+
const newCoreImports = ts.factory.updateNamedImports(coreImports, [
41+
...coreImports.elements.filter((current) => phaseEnum !== current),
42+
]);
43+
changeTracker.replaceNode(coreImports, newCoreImports);
44+
ts.forEachChild(sourceFile, function visit(node: ts.Node) {
45+
ts.forEachChild(node, visit);
3346

34-
// Check if this is a function call of `afterRender` or `afterNextRender`.
35-
if (
36-
ts.isCallExpression(node) &&
37-
ts.isIdentifier(node.expression) &&
38-
AFTER_RENDER_FNS.has(getImportOfIdentifier(typeChecker, node.expression)?.name || '')
39-
) {
40-
let phase: string | undefined;
41-
const [callback, options] = node.arguments;
42-
// Check if any `AfterRenderOptions` options were specified.
43-
if (ts.isObjectLiteralExpression(options)) {
44-
const phaseProp = options.properties.find((p) => p.name?.getText() === 'phase');
45-
// Check if the `phase` options is set.
46-
if (
47-
phaseProp &&
48-
ts.isPropertyAssignment(phaseProp) &&
49-
ts.isPropertyAccessExpression(phaseProp.initializer) &&
50-
phaseProp.initializer.expression.getText() === AFTER_RENDER_PHASE_ENUM
51-
) {
52-
phaseProp.initializer.expression;
53-
phase = phaseProp.initializer.name.getText();
54-
// Remove the `phase` option.
55-
if (options.properties.length === 1) {
56-
changeTracker.removeNode(options);
57-
} else {
58-
const newOptions = ts.factory.createObjectLiteralExpression(
59-
options.properties.filter((p) => p !== phaseProp),
60-
);
61-
changeTracker.replaceNode(options, newOptions);
62-
}
47+
// Check if this is a function call of `afterRender` or `afterNextRender`.
48+
if (
49+
ts.isCallExpression(node) &&
50+
ts.isIdentifier(node.expression) &&
51+
AFTER_RENDER_FNS.has(getImportOfIdentifier(typeChecker, node.expression)?.name || '')
52+
) {
53+
let phase: string | undefined;
54+
const [callback, options] = node.arguments;
55+
// Check if any `AfterRenderOptions` options were specified.
56+
if (ts.isObjectLiteralExpression(options)) {
57+
const phaseProp = options.properties.find((p) => p.name?.getText() === 'phase');
58+
// Check if the `phase` options is set.
59+
if (
60+
phaseProp &&
61+
ts.isPropertyAssignment(phaseProp) &&
62+
ts.isPropertyAccessExpression(phaseProp.initializer) &&
63+
phaseProp.initializer.expression.getText() === AFTER_RENDER_PHASE_ENUM
64+
) {
65+
phaseProp.initializer.expression;
66+
phase = phaseProp.initializer.name.getText();
67+
// Remove the `phase` option.
68+
if (options.properties.length === 1) {
69+
changeTracker.removeNode(options);
70+
} else {
71+
const newOptions = ts.factory.createObjectLiteralExpression(
72+
options.properties.filter((p) => p !== phaseProp),
73+
);
74+
changeTracker.replaceNode(options, newOptions);
6375
}
6476
}
65-
// If we found a phase, update the callback.
66-
if (phase) {
67-
phase = phase.substring(0, 1).toLocaleLowerCase() + phase.substring(1);
68-
const spec = ts.factory.createObjectLiteralExpression([
69-
ts.factory.createPropertyAssignment(ts.factory.createIdentifier(phase), callback),
70-
]);
71-
changeTracker.replaceNode(callback, spec);
72-
}
7377
}
74-
});
75-
}
78+
// If we found a phase, update the callback.
79+
if (phase) {
80+
phase = phase.substring(0, 1).toLocaleLowerCase() + phase.substring(1);
81+
const spec = ts.factory.createObjectLiteralExpression([
82+
ts.factory.createPropertyAssignment(ts.factory.createIdentifier(phase), callback),
83+
]);
84+
changeTracker.replaceNode(callback, spec);
85+
}
86+
}
87+
});
7688

7789
// Write the changes.
7890
for (const changesInFile of changeTracker.recordChanges().values()) {

‎packages/core/schematics/test/after_render_phase_spec.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ describe('afterRender phase migration', () => {
8181

8282
const content = tree.readContent('/index.ts').replace(/\s+/g, ' ');
8383
expect(content).not.toContain('AfterRenderPhase');
84+
expect(content).toContain("import { Directive, afterRender } from '@angular/core';");
8485
expect(content).toContain(`afterRender({ read: () => { console.log('read'); } }, );`);
8586
});
8687

@@ -106,6 +107,7 @@ describe('afterRender phase migration', () => {
106107

107108
const content = tree.readContent('/index.ts').replace(/\s+/g, ' ');
108109
expect(content).not.toContain('AfterRenderPhase');
110+
expect(content).toContain("import { Directive, afterNextRender } from '@angular/core';");
109111
expect(content).toContain(
110112
`afterNextRender({ earlyRead: () => { console.log('earlyRead'); } }, );`,
111113
);
@@ -148,7 +150,7 @@ describe('afterRender phase migration', () => {
148150
writeFile(
149151
'/index.ts',
150152
`
151-
import { AfterRenderPhase, Directive, Injector, afterRender, inject } from '@angular/core';
153+
import { Directive, Injector, afterRender, AfterRenderPhase, inject } from '@angular/core';
152154
153155
@Directive({
154156
selector: '[someDirective]'
@@ -169,6 +171,10 @@ describe('afterRender phase migration', () => {
169171

170172
await runMigration();
171173
const content = tree.readContent('/index.ts').replace(/\s+/g, ' ');
174+
expect(content).not.toContain('AfterRenderPhase');
175+
expect(content).toContain(
176+
"import { Directive, Injector, afterRender, inject } from '@angular/core';",
177+
);
172178
expect(content).toContain(
173179
`afterRender({ earlyRead: () => { console.log('earlyRead'); } }, { injector: this.injector });`,
174180
);

0 commit comments

Comments
 (0)