Skip to content

Commit 45dc998

Browse files
clydinclydin
committed
refactor(@angular/cli): migrate MCP zoneless tool to host abstraction
Migrate the zoneless migration tool to use the Host abstraction for file system operations including reading files, checking stats, and globbing. This ensures that the tool adheres to the allowed roots configured in the MCP server. By removing direct dependencies on native Node.js fs modules, the tool now benefits from the centralized injection and restriction model provided by the Host implementation.
1 parent c9f4081 commit 45dc998

6 files changed

Lines changed: 55 additions & 41 deletions

File tree

‎packages/angular/cli/src/commands/mcp/tools/onpush-zoneless-migration/migrate-single-file.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import type { RequestHandlerExtra } from '@modelcontextprotocol/sdk/shared/protocol';
1010
import type { ServerNotification, ServerRequest } from '@modelcontextprotocol/sdk/types';
1111
import type { SourceFile } from 'typescript';
12+
import type { Host } from '../../host';
1213
import { analyzeForUnsupportedZoneUses } from './analyze-for-unsupported-zone-uses';
1314
import { migrateTestFile } from './migrate-test-file';
1415
import { generateZonelessMigrationInstructionsForComponent } from './prompts';
@@ -20,12 +21,13 @@ const supportedStrategies: ReadonlySet<string> = new Set(['OnPush', 'Default', '
2021

2122
export async function migrateSingleFile(
2223
sourceFile: SourceFile,
24+
host: Host,
2325
extras: RequestHandlerExtra<ServerRequest, ServerNotification>,
2426
): Promise<MigrationResponse | null> {
2527
const testBedSpecifier = await getImportSpecifier(sourceFile, '@angular/core/testing', 'TestBed');
2628
const isTestFile = sourceFile.fileName.endsWith('.spec.ts') || !!testBedSpecifier;
2729
if (isTestFile) {
28-
return migrateTestFile(sourceFile);
30+
return migrateTestFile(sourceFile, host);
2931
}
3032

3133
const unsupportedZoneUseResponse = await analyzeForUnsupportedZoneUses(sourceFile);

‎packages/angular/cli/src/commands/mcp/tools/onpush-zoneless-migration/migrate-single-file_spec.ts‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import type { RequestHandlerExtra } from '@modelcontextprotocol/sdk/shared/protocol';
1010
import type { ServerNotification, ServerRequest } from '@modelcontextprotocol/sdk/types';
1111
import ts from 'typescript';
12+
import { createMockHost } from '../../testing/test-utils';
1213
import { migrateSingleFile } from './migrate-single-file';
1314

1415
const fakeExtras = {
@@ -17,11 +18,13 @@ const fakeExtras = {
1718
} as unknown as RequestHandlerExtra<ServerRequest, ServerNotification>;
1819

1920
describe('migrateSingleFile', () => {
21+
const mockHost = createMockHost();
22+
2023
it('should identify test files by extension', async () => {
2124
const fileName = 'test.spec.ts';
2225
const sourceFile = ts.createSourceFile(fileName, '', ts.ScriptTarget.ESNext, true);
2326

24-
const result = await migrateSingleFile(sourceFile, fakeExtras);
27+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
2528

2629
expect(result?.content[0].text).toContain(
2730
'The test file `test.spec.ts` is not yet configured for zoneless change detection.' +
@@ -34,7 +37,7 @@ describe('migrateSingleFile', () => {
3437
const content = `import { TestBed } from '@angular/core/testing';`;
3538
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
3639

37-
const result = await migrateSingleFile(sourceFile, fakeExtras);
40+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
3841

3942
expect(result?.content[0].text).toContain(
4043
'The test file `test.ts` is not yet configured for zoneless change detection.' +
@@ -59,7 +62,7 @@ describe('migrateSingleFile', () => {
5962
`;
6063
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
6164

62-
const result = await migrateSingleFile(sourceFile, fakeExtras);
65+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
6366

6467
expect(result?.content[0].text).toContain(
6568
'The component uses NgZone APIs that are incompatible with zoneless applications',
@@ -80,7 +83,7 @@ describe('migrateSingleFile', () => {
8083
`;
8184
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
8285

83-
const result = await migrateSingleFile(sourceFile, fakeExtras);
86+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
8487

8588
expect(result).toBeNull();
8689
});
@@ -99,7 +102,7 @@ describe('migrateSingleFile', () => {
99102
`;
100103
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
101104

102-
const result = await migrateSingleFile(sourceFile, fakeExtras);
105+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
103106

104107
expect(result).toBeNull();
105108
});
@@ -117,7 +120,7 @@ describe('migrateSingleFile', () => {
117120
`;
118121
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
119122

120-
const result = await migrateSingleFile(sourceFile, fakeExtras);
123+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
121124

122125
expect(result?.content[0].text).toContain(
123126
'The component does not currently use a change detection strategy, which means it may rely on Zone.js',
@@ -134,7 +137,7 @@ describe('migrateSingleFile', () => {
134137
`;
135138
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
136139

137-
const result = await migrateSingleFile(sourceFile, fakeExtras);
140+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
138141

139142
expect(result).toBeNull();
140143
});
@@ -144,7 +147,7 @@ describe('migrateSingleFile', () => {
144147
const content = ``;
145148
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
146149

147-
const result = await migrateSingleFile(sourceFile, fakeExtras);
150+
const result = await migrateSingleFile(sourceFile, mockHost, fakeExtras);
148151

149152
expect(result).toBeNull();
150153
});

‎packages/angular/cli/src/commands/mcp/tools/onpush-zoneless-migration/migrate-test-file.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,19 +6,21 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import { existsSync, readFileSync } from 'node:fs';
10-
import { glob } from 'node:fs/promises';
11-
import { dirname, join } from 'node:path';
9+
import { type Host } from '../../host';
10+
import { join } from 'node:path';
1211
import type { SourceFile } from 'typescript';
1312
import { findAngularJsonDir } from '../../workspace-utils';
1413
import { createFixResponseForZoneTests, createProvideZonelessForTestsSetupPrompt } from './prompts';
1514
import { loadTypescript } from './ts-utils';
1615
import { MigrationResponse } from './types';
1716

18-
export async function migrateTestFile(sourceFile: SourceFile): Promise<MigrationResponse | null> {
17+
export async function migrateTestFile(
18+
sourceFile: SourceFile,
19+
host: Host,
20+
): Promise<MigrationResponse | null> {
1921
const ts = await loadTypescript();
2022
// Check if tests use zoneless either by default through `initTestEnvironment` or by explicitly calling `provideZonelessChangeDetection`.
21-
let testsUseZonelessChangeDetection = await searchForGlobalZoneless(sourceFile.fileName);
23+
let testsUseZonelessChangeDetection = await searchForGlobalZoneless(sourceFile.fileName, host);
2224
if (!testsUseZonelessChangeDetection) {
2325
ts.forEachChild(sourceFile, function visit(node) {
2426
if (
@@ -42,18 +44,19 @@ export async function migrateTestFile(sourceFile: SourceFile): Promise<Migration
4244
return createFixResponseForZoneTests(sourceFile);
4345
}
4446

45-
export async function searchForGlobalZoneless(startPath: string): Promise<boolean> {
46-
const angularJsonDir = findAngularJsonDir(startPath);
47+
export async function searchForGlobalZoneless(startPath: string, host: Host): Promise<boolean> {
48+
const angularJsonDir = findAngularJsonDir(startPath, host);
4749
if (!angularJsonDir) {
4850
// Cannot determine project root, fallback to original behavior or assume false.
4951
// For now, let's assume no global setup if angular.json is not found.
5052
return false;
5153
}
5254

5355
try {
54-
const files = glob(`${angularJsonDir}/**/*.ts`);
56+
const files = host.glob('**/*.ts', { cwd: angularJsonDir });
5557
for await (const file of files) {
56-
const content = readFileSync(file, 'utf-8');
58+
const fullPath = join(file.parentPath, file.name);
59+
const content = await host.readFile(fullPath, 'utf-8');
5760
if (
5861
content.includes('initTestEnvironment') &&
5962
content.includes('provideZonelessChangeDetection')

‎packages/angular/cli/src/commands/mcp/tools/onpush-zoneless-migration/migrate-test-file_spec.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,16 @@
77
*/
88

99
import ts from 'typescript';
10+
import { createMockHost } from '../../testing/test-utils';
1011
import { migrateTestFile } from './migrate-test-file';
1112

1213
describe('migrateTestFile', () => {
14+
const mockHost = createMockHost();
1315
it('should return setup prompt when zoneless is not detected', async () => {
1416
const fileName = 'test.spec.ts';
1517
const sourceFile = ts.createSourceFile(fileName, '', ts.ScriptTarget.ESNext, true);
1618

17-
const result = await migrateTestFile(sourceFile);
19+
const result = await migrateTestFile(sourceFile, mockHost);
1820

1921
expect(result?.content[0].text).toContain(
2022
'The test file `test.spec.ts` is not yet configured for zoneless change detection.',
@@ -33,7 +35,7 @@ describe('migrateTestFile', () => {
3335
`;
3436
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
3537

36-
const result = await migrateTestFile(sourceFile);
38+
const result = await migrateTestFile(sourceFile, mockHost);
3739

3840
expect(result).toBeNull();
3941
});
@@ -60,7 +62,7 @@ describe('migrateTestFile', () => {
6062
`;
6163
const sourceFile = ts.createSourceFile(fileName, content, ts.ScriptTarget.ESNext, true);
6264

63-
const result = await migrateTestFile(sourceFile);
65+
const result = await migrateTestFile(sourceFile, mockHost);
6466

6567
expect(result?.content[0].text).toContain(
6668
'You must refactor these tests to work in a zoneless environment and remove the `provideZoneChangeDetection` calls.',

‎packages/angular/cli/src/commands/mcp/tools/onpush-zoneless-migration/ts-utils.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import { readFileSync } from 'node:fs';
9+
import type { Host } from '../../host';
1010
import type ts from 'typescript';
1111

1212
let typescriptModule: typeof ts;
@@ -116,8 +116,8 @@ export function findImportSpecifier(
116116
}
117117

118118
/** Creates a TypeScript source file from a file path. */
119-
export async function createSourceFile(file: string) {
120-
const content = readFileSync(file, 'utf8');
119+
export async function createSourceFile(file: string, host: Host) {
120+
const content = await host.readFile(file, 'utf8');
121121

122122
const ts = await loadTypescript();
123123

‎packages/angular/cli/src/commands/mcp/tools/onpush-zoneless-migration/zoneless-migration.ts‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88

99
import type { RequestHandlerExtra } from '@modelcontextprotocol/sdk/shared/protocol';
1010
import type { ServerNotification, ServerRequest } from '@modelcontextprotocol/sdk/types';
11-
import { existsSync, statSync } from 'node:fs';
12-
import { glob } from 'node:fs/promises';
11+
import type { Host } from '../../host';
12+
import { join } from 'node:path';
1313
import type { SourceFile } from 'typescript';
1414
import { z } from 'zod';
1515
import { declareTool } from '../tool-registry';
@@ -61,19 +61,20 @@ most important action to take in the migration journey.
6161
),
6262
},
6363
factory:
64-
() =>
64+
({ host }) =>
6565
({ fileOrDirPath }, requestHandlerExtra) =>
66-
registerZonelessMigrationTool(fileOrDirPath, requestHandlerExtra),
66+
registerZonelessMigrationTool(fileOrDirPath, host, requestHandlerExtra),
6767
});
6868

6969
export async function registerZonelessMigrationTool(
7070
fileOrDirPath: string,
71+
host: Host,
7172
extras: RequestHandlerExtra<ServerRequest, ServerNotification>,
7273
) {
7374
let filesWithComponents, componentTestFiles, zoneFiles, categorizationErrors;
7475
try {
7576
({ filesWithComponents, componentTestFiles, zoneFiles, categorizationErrors } =
76-
await discoverAndCategorizeFiles(fileOrDirPath, extras));
77+
await discoverAndCategorizeFiles(fileOrDirPath, host, extras));
7778
} catch (e) {
7879
return createResponse(
7980
`Error: Could not access the specified path. Please ensure the following path is correct ` +
@@ -97,15 +98,15 @@ export async function registerZonelessMigrationTool(
9798
: Array.from(filesWithComponents);
9899

99100
for (const file of rankedFiles) {
100-
const result = await migrateSingleFile(file, extras);
101+
const result = await migrateSingleFile(file, host, extras);
101102
if (result !== null) {
102103
return result;
103104
}
104105
}
105106
}
106107

107108
for (const file of componentTestFiles) {
108-
const result = await migrateTestFile(file);
109+
const result = await migrateTestFile(file, host);
109110
if (result !== null) {
110111
return result;
111112
}
@@ -124,6 +125,7 @@ export async function registerZonelessMigrationTool(
124125

125126
async function discoverAndCategorizeFiles(
126127
fileOrDirPath: string,
128+
host: Host,
127129
extras: RequestHandlerExtra<ServerRequest, ServerNotification>,
128130
) {
129131
const filePaths: string[] = [];
@@ -134,19 +136,20 @@ async function discoverAndCategorizeFiles(
134136

135137
let isDirectory: boolean;
136138
try {
137-
isDirectory = statSync(fileOrDirPath).isDirectory();
139+
isDirectory = (await host.stat(fileOrDirPath)).isDirectory();
138140
} catch (e) {
139141
// Re-throw to be handled by the main function as a user input error
140142
throw new Error(`Failed to access path: ${fileOrDirPath}`, { cause: e });
141143
}
142144

143145
if (isDirectory) {
144-
for await (const file of glob(`${fileOrDirPath}/**/*.ts`)) {
145-
filePaths.push(file);
146+
const files = host.glob('**/*.ts', { cwd: fileOrDirPath });
147+
for await (const file of files) {
148+
filePaths.push(join(file.parentPath, file.name));
146149
}
147150
} else {
148151
filePaths.push(fileOrDirPath);
149-
const maybeTestFile = await getTestFilePath(fileOrDirPath);
152+
const maybeTestFile = await getTestFilePath(fileOrDirPath, host);
150153
if (maybeTestFile) {
151154
// Eagerly add the test file path for categorization.
152155
filePaths.push(maybeTestFile);
@@ -159,8 +162,8 @@ async function discoverAndCategorizeFiles(
159162
const batch = filesToProcess.splice(0, CONCURRENCY_LIMIT);
160163
const results = await Promise.allSettled(
161164
batch.map(async (filePath) => {
162-
const sourceFile = await createSourceFile(filePath);
163-
await categorizeFile(sourceFile, extras, {
165+
const sourceFile = await createSourceFile(filePath, host);
166+
await categorizeFile(sourceFile, host, extras, {
164167
filesWithComponents,
165168
componentTestFiles,
166169
zoneFiles,
@@ -183,6 +186,7 @@ async function discoverAndCategorizeFiles(
183186

184187
async function categorizeFile(
185188
sourceFile: SourceFile,
189+
host: Host,
186190
extras: RequestHandlerExtra<ServerRequest, ServerNotification>,
187191
categorizedFiles: {
188192
filesWithComponents: Set<SourceFile>;
@@ -214,9 +218,9 @@ async function categorizeFile(
214218
);
215219
}
216220

217-
const testFilePath = await getTestFilePath(sourceFile.fileName);
221+
const testFilePath = await getTestFilePath(sourceFile.fileName, host);
218222
if (testFilePath) {
219-
componentTestFiles.add(await createSourceFile(testFilePath));
223+
componentTestFiles.add(await createSourceFile(testFilePath, host));
220224
}
221225
} else if (zoneSpecifier) {
222226
zoneFiles.add(sourceFile);
@@ -271,9 +275,9 @@ async function rankComponentFilesForMigration(
271275
return componentFiles; // Fallback to original order if the response fails
272276
}
273277

274-
async function getTestFilePath(filePath: string): Promise<string | undefined> {
278+
async function getTestFilePath(filePath: string, host: Host): Promise<string | undefined> {
275279
const testFilePath = filePath.replace(/\.ts$/, '.spec.ts');
276-
if (existsSync(testFilePath)) {
280+
if (host.existsSync(testFilePath)) {
277281
return testFilePath;
278282
}
279283

0 commit comments

Comments
 (0)