Skip to content

Commit

Permalink
fix(core): properly remove imports in the afterRender phase migration
Browse files Browse the repository at this point in the history
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';
```
  • Loading branch information
cexbrayat committed Jun 19, 2024
1 parent 0bd55a6 commit 7c7ba66
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 47 deletions.
104 changes: 58 additions & 46 deletions packages/core/schematics/migrations/after-render-phase/migration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@

import ts from 'typescript';
import {ChangeTracker} from '../../utils/change_tracker';
import {getImportOfIdentifier, getImportSpecifier} from '../../utils/typescript/imports';
import {
getImportOfIdentifier,
getImportSpecifier,
getNamedImports,
} from '../../utils/typescript/imports';

const CORE = '@angular/core';
const AFTER_RENDER_PHASE_ENUM = 'AfterRenderPhase';
Expand All @@ -22,57 +26,65 @@ export function migrateFile(
rewriteFn: RewriteFn,
) {
const changeTracker = new ChangeTracker(ts.createPrinter());
// Check if there are any imports of the `AfterRenderPhase` enum.
const coreImports = getNamedImports(sourceFile, CORE);
if (!coreImports) {
return;
}
const phaseEnum = getImportSpecifier(sourceFile, CORE, AFTER_RENDER_PHASE_ENUM);
if (!phaseEnum) {
return;
}

// Check if there are any imports of the `AfterRenderPhase` enum.
if (phaseEnum) {
// Remove the `AfterRenderPhase` enum import.
changeTracker.removeNode(phaseEnum);
ts.forEachChild(sourceFile, function visit(node: ts.Node) {
ts.forEachChild(node, visit);
// Remove the `AfterRenderPhase` enum import.
const newCoreImports = ts.factory.updateNamedImports(coreImports, [
...coreImports.elements.filter((current) => phaseEnum !== current),
]);
changeTracker.replaceNode(coreImports, newCoreImports);
ts.forEachChild(sourceFile, function visit(node: ts.Node) {
ts.forEachChild(node, visit);

// Check if this is a function call of `afterRender` or `afterNextRender`.
if (
ts.isCallExpression(node) &&
ts.isIdentifier(node.expression) &&
AFTER_RENDER_FNS.has(getImportOfIdentifier(typeChecker, node.expression)?.name || '')
) {
let phase: string | undefined;
const [callback, options] = node.arguments;
// Check if any `AfterRenderOptions` options were specified.
if (ts.isObjectLiteralExpression(options)) {
const phaseProp = options.properties.find((p) => p.name?.getText() === 'phase');
// Check if the `phase` options is set.
if (
phaseProp &&
ts.isPropertyAssignment(phaseProp) &&
ts.isPropertyAccessExpression(phaseProp.initializer) &&
phaseProp.initializer.expression.getText() === AFTER_RENDER_PHASE_ENUM
) {
phaseProp.initializer.expression;
phase = phaseProp.initializer.name.getText();
// Remove the `phase` option.
if (options.properties.length === 1) {
changeTracker.removeNode(options);
} else {
const newOptions = ts.factory.createObjectLiteralExpression(
options.properties.filter((p) => p !== phaseProp),
);
changeTracker.replaceNode(options, newOptions);
}
// Check if this is a function call of `afterRender` or `afterNextRender`.
if (
ts.isCallExpression(node) &&
ts.isIdentifier(node.expression) &&
AFTER_RENDER_FNS.has(getImportOfIdentifier(typeChecker, node.expression)?.name || '')
) {
let phase: string | undefined;
const [callback, options] = node.arguments;
// Check if any `AfterRenderOptions` options were specified.
if (ts.isObjectLiteralExpression(options)) {
const phaseProp = options.properties.find((p) => p.name?.getText() === 'phase');
// Check if the `phase` options is set.
if (
phaseProp &&
ts.isPropertyAssignment(phaseProp) &&
ts.isPropertyAccessExpression(phaseProp.initializer) &&
phaseProp.initializer.expression.getText() === AFTER_RENDER_PHASE_ENUM
) {
phaseProp.initializer.expression;
phase = phaseProp.initializer.name.getText();
// Remove the `phase` option.
if (options.properties.length === 1) {
changeTracker.removeNode(options);
} else {
const newOptions = ts.factory.createObjectLiteralExpression(
options.properties.filter((p) => p !== phaseProp),
);
changeTracker.replaceNode(options, newOptions);
}
}
// If we found a phase, update the callback.
if (phase) {
phase = phase.substring(0, 1).toLocaleLowerCase() + phase.substring(1);
const spec = ts.factory.createObjectLiteralExpression([
ts.factory.createPropertyAssignment(ts.factory.createIdentifier(phase), callback),
]);
changeTracker.replaceNode(callback, spec);
}
}
});
}
// If we found a phase, update the callback.
if (phase) {
phase = phase.substring(0, 1).toLocaleLowerCase() + phase.substring(1);
const spec = ts.factory.createObjectLiteralExpression([
ts.factory.createPropertyAssignment(ts.factory.createIdentifier(phase), callback),
]);
changeTracker.replaceNode(callback, spec);
}
}
});

// Write the changes.
for (const changesInFile of changeTracker.recordChanges().values()) {
Expand Down
8 changes: 7 additions & 1 deletion packages/core/schematics/test/after_render_phase_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ describe('afterRender phase migration', () => {

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

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

const content = tree.readContent('/index.ts').replace(/\s+/g, ' ');
expect(content).not.toContain('AfterRenderPhase');
expect(content).toContain("import { Directive, afterNextRender } from '@angular/core';");
expect(content).toContain(
`afterNextRender({ earlyRead: () => { console.log('earlyRead'); } }, );`,
);
Expand Down Expand Up @@ -148,7 +150,7 @@ describe('afterRender phase migration', () => {
writeFile(
'/index.ts',
`
import { AfterRenderPhase, Directive, Injector, afterRender, inject } from '@angular/core';
import { Directive, Injector, afterRender, AfterRenderPhase, inject } from '@angular/core';
@Directive({
selector: '[someDirective]'
Expand All @@ -169,6 +171,10 @@ describe('afterRender phase migration', () => {

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

0 comments on commit 7c7ba66

Please sign in to comment.