Skip to content

Commit 8459ee4

Browse files
cexbrayatAndrewKushnir
cexbrayat
authored and
AndrewKushnir
committed
fix(migrations): handle more cases in HttpClientModule migration (#55640)
This commit handles two cases that were breaking applications when using the new migration: - tests using `HttpClientModule` in `TestBed.configureTestingModule` were broken as the import was removed, but the module is still present in the test configuration. It now properly adds `provideHttpClient(withInterceptorsFromDi())` and related imports to the test. - tests using `HttpClientTestingModule` were migrated to use `provideHttpClient(withInterceptorsFromDi())` but the necessary imports were not added. They are now added by the migration. PR Close #55640
1 parent 3de1bc6 commit 8459ee4

File tree

4 files changed

+199
-62
lines changed

4 files changed

+199
-62
lines changed

‎packages/core/schematics/migrations/http-providers/README.md‎

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ This migration updates any `@NgModule`, `@Component`, `@Directive` that imports
1313
import { HttpClientModule, HttpClientJsonpModule, HttpClientXsrfModule } from '@angular/common/http';
1414

1515
@NgModule({
16-
imports: [CommonModule, HttpClientModule,HttpClientJsonpModule, HttpClientXsrfModule)],
16+
imports: [CommonModule, HttpClientModule, HttpClientJsonpModule, HttpClientXsrfModule]
1717
})
1818
export class AppModule {}
1919
```
@@ -33,30 +33,61 @@ export class AppModule {}
3333

3434
#### Before
3535

36+
```ts
37+
import { HttpClientTestingModule } from '@angular/common/http/testing';
38+
39+
describe('some test', () => {
40+
41+
it('...', () => {
42+
TestBed.configureTestingModule({
43+
imports: [HttpClientTestingModule]
44+
})
45+
})
46+
})
3647
```
37-
import { HttpClientTestingModule } from '@not-angular/common/http/testing';
3848

39-
describe('some test') {
49+
#### After
50+
51+
```ts
52+
import { provideHttpClientTesting } from '@angular/common/http/testing';
53+
import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http';
54+
55+
describe('some test', () => {
4056

4157
it('...', () => {
4258
TestBed.configureTestingModule({
43-
imports: [HttpClientTestingModule],
44-
});
59+
providers: [provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting()]
60+
})
4561
})
46-
}
62+
})
4763
```
4864

4965
#### Before
5066

67+
```ts
68+
import { HttpClientTesting } from '@angular/common/http';
69+
70+
describe('some test', () => {
71+
72+
it('...', () => {
73+
TestBed.configureTestingModule({
74+
imports: [HttpClientTesting],
75+
})
76+
})
77+
});
5178
```
52-
import { provideHttpClientTesting } from '@not-angular/common/http/testing';
5379

54-
describe('some test') {
80+
#### After
81+
82+
```ts
83+
import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http';
84+
85+
describe('some test', () => {
5586

5687
it('...', () => {
5788
TestBed.configureTestingModule({
58-
providers: [provideHttpClientTesting()],
59-
});
89+
providers: [provideHttpClient(withInterceptorsFromDi())]
90+
})
6091
})
61-
}
92+
})
6293
```

‎packages/core/schematics/migrations/http-providers/utils.ts‎

Lines changed: 124 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,13 @@ export function migrateFile(
6767
});
6868
}
6969

70-
migrateTestingModuleImports(node, commonHttpTestingIdentifiers, addedImports, changeTracker);
70+
migrateTestingModuleImports(
71+
node,
72+
commonHttpIdentifiers,
73+
commonHttpTestingIdentifiers,
74+
addedImports,
75+
changeTracker,
76+
);
7177
});
7278

7379
// Imports are for the whole file
@@ -90,6 +96,13 @@ export function migrateFile(
9096
]);
9197
changeTracker.replaceNode(commonHttpImports, newImports);
9298
}
99+
// If there are no imports for common/http, and we need to add some
100+
else if (addedImports.get(COMMON_HTTP)?.size) {
101+
// Then we add a new import statement for common/http
102+
addedImports.get(COMMON_HTTP)?.forEach((entry) => {
103+
changeTracker.addImport(sourceFile, entry, COMMON_HTTP);
104+
});
105+
}
93106

94107
// Remove the HttpModules imports from common/http/testing
95108
const commonHttpTestingImports = getNamedImports(sourceFile, COMMON_HTTP_TESTING);
@@ -156,22 +169,23 @@ function migrateDecorator(
156169
const addedProviders = new Set<ts.CallExpression>();
157170

158171
// Handle the different imported Http modules
172+
const commonHttpAddedImports = addedImports.get(COMMON_HTTP);
159173
if (importedModules.client) {
160-
addedImports.get(COMMON_HTTP)?.add(WITH_INTERCEPTORS_FROM_DI);
174+
commonHttpAddedImports?.add(WITH_INTERCEPTORS_FROM_DI);
161175
addedProviders.add(createCallExpression(WITH_INTERCEPTORS_FROM_DI));
162176
}
163177
if (importedModules.clientJsonp) {
164-
addedImports.get(COMMON_HTTP)?.add(WITH_JSONP_SUPPORT);
178+
commonHttpAddedImports?.add(WITH_JSONP_SUPPORT);
165179
addedProviders.add(createCallExpression(WITH_JSONP_SUPPORT));
166180
}
167181
if (importedModules.xsrf) {
168182
// HttpClientXsrfModule is the only module with Class methods.
169183
// They correspond to different provider functions
170184
if (importedModules.xsrfOptions === 'disable') {
171-
addedImports.get(COMMON_HTTP)?.add(WITH_NOXSRF_PROTECTION);
185+
commonHttpAddedImports?.add(WITH_NOXSRF_PROTECTION);
172186
addedProviders.add(createCallExpression(WITH_NOXSRF_PROTECTION));
173187
} else {
174-
addedImports.get(COMMON_HTTP)?.add(WITH_XSRF_CONFIGURATION);
188+
commonHttpAddedImports?.add(WITH_XSRF_CONFIGURATION);
175189
addedProviders.add(
176190
createCallExpression(
177191
WITH_XSRF_CONFIGURATION,
@@ -192,20 +206,23 @@ function migrateDecorator(
192206
]);
193207

194208
// Adding the new providers
195-
addedImports.get(COMMON_HTTP)?.add(PROVIDE_HTTP_CLIENT);
209+
commonHttpAddedImports?.add(PROVIDE_HTTP_CLIENT);
196210
const providers = getProvidersFromLiteralExpr(metadata);
197211
const provideHttpExpr = createCallExpression(PROVIDE_HTTP_CLIENT, [...addedProviders]);
198212

199213
let newProviders: ts.ArrayLiteralExpression;
200214
if (!providers) {
201-
// No existing providers, we add an property to the literal
215+
// No existing providers, we add a property to the literal
202216
newProviders = ts.factory.createArrayLiteralExpression([provideHttpExpr]);
203217
} else {
204218
// We add the provider to the existing provider array
205-
newProviders = ts.factory.createArrayLiteralExpression([
206-
...providers.elements,
207-
provideHttpExpr,
208-
]);
219+
newProviders = ts.factory.updateArrayLiteralExpression(
220+
providers,
221+
ts.factory.createNodeArray(
222+
[...providers.elements, provideHttpExpr],
223+
providers.elements.hasTrailingComma,
224+
),
225+
);
209226
}
210227

211228
// Replacing the existing decorator with the new one (with the new imports and providers)
@@ -219,6 +236,7 @@ function migrateDecorator(
219236

220237
function migrateTestingModuleImports(
221238
node: ts.Node,
239+
commonHttpIdentifiers: Set<string>,
222240
commonHttpTestingIdentifiers: Set<string>,
223241
addedImports: Map<string, Set<string>>,
224242
changeTracker: ChangeTracker,
@@ -248,51 +266,98 @@ function migrateTestingModuleImports(
248266
return;
249267
}
250268

251-
// Does the imports array contain the HttpClientTestingModule?
252-
const httpClientTesting = importsArray.elements.find(
253-
(elt) => elt.getText() === HTTP_CLIENT_TESTING_MODULE,
254-
);
255-
if (!httpClientTesting || !commonHttpTestingIdentifiers.has(HTTP_CLIENT_TESTING_MODULE)) {
256-
return;
257-
}
269+
const commonHttpAddedImports = addedImports.get(COMMON_HTTP);
258270

259-
addedImports.get(COMMON_HTTP_TESTING)?.add(PROVIDE_HTTP_CLIENT_TESTING);
271+
// Does the imports array contain the HttpClientModule?
272+
const httpClient = importsArray.elements.find((elt) => elt.getText() === HTTP_CLIENT_MODULE);
273+
if (httpClient && commonHttpIdentifiers.has(HTTP_CLIENT_MODULE)) {
274+
// We add the imports for provideHttpClient(withInterceptorsFromDi())
275+
commonHttpAddedImports?.add(PROVIDE_HTTP_CLIENT);
276+
commonHttpAddedImports?.add(WITH_INTERCEPTORS_FROM_DI);
260277

261-
const newImports = ts.factory.createArrayLiteralExpression([
262-
...importsArray.elements.filter((item) => item !== httpClientTesting),
263-
]);
278+
const newImports = ts.factory.createArrayLiteralExpression([
279+
...importsArray.elements.filter((item) => item !== httpClient),
280+
]);
264281

265-
const provideHttpClient = createCallExpression(PROVIDE_HTTP_CLIENT, [
266-
createCallExpression(WITH_INTERCEPTORS_FROM_DI),
267-
]);
268-
const provideHttpClientTesting = createCallExpression(PROVIDE_HTTP_CLIENT_TESTING);
282+
const provideHttpClient = createCallExpression(PROVIDE_HTTP_CLIENT, [
283+
createCallExpression(WITH_INTERCEPTORS_FROM_DI),
284+
]);
269285

270-
// Adding the new providers
271-
const providers = getProvidersFromLiteralExpr(configureTestingModuleArgs);
286+
// Adding the new provider
287+
const providers = getProvidersFromLiteralExpr(configureTestingModuleArgs);
272288

273-
let newProviders: ts.ArrayLiteralExpression;
274-
if (!providers) {
275-
// No existing providers, we add an property to the literal
276-
newProviders = ts.factory.createArrayLiteralExpression([
277-
provideHttpClient,
278-
provideHttpClientTesting,
289+
let newProviders: ts.ArrayLiteralExpression;
290+
if (!providers) {
291+
// No existing providers, we add a property to the literal
292+
newProviders = ts.factory.createArrayLiteralExpression([provideHttpClient]);
293+
} else {
294+
// We add the provider to the existing provider array
295+
newProviders = ts.factory.updateArrayLiteralExpression(
296+
providers,
297+
ts.factory.createNodeArray(
298+
[...providers.elements, provideHttpClient],
299+
providers.elements.hasTrailingComma,
300+
),
301+
);
302+
}
303+
304+
// Replacing the existing configuration with the new one (with the new imports and providers)
305+
const newTestingModuleArgs = updateTestBedConfiguration(
306+
configureTestingModuleArgs,
307+
newImports,
308+
newProviders,
309+
);
310+
changeTracker.replaceNode(configureTestingModuleArgs, newTestingModuleArgs);
311+
}
312+
313+
// Does the imports array contain the HttpClientTestingModule?
314+
const httpClientTesting = importsArray.elements.find(
315+
(elt) => elt.getText() === HTTP_CLIENT_TESTING_MODULE,
316+
);
317+
if (httpClientTesting && commonHttpTestingIdentifiers.has(HTTP_CLIENT_TESTING_MODULE)) {
318+
// We add the imports for provideHttpClient(withInterceptorsFromDi()) and provideHttpClientTesting()
319+
commonHttpAddedImports?.add(PROVIDE_HTTP_CLIENT);
320+
commonHttpAddedImports?.add(WITH_INTERCEPTORS_FROM_DI);
321+
addedImports.get(COMMON_HTTP_TESTING)?.add(PROVIDE_HTTP_CLIENT_TESTING);
322+
323+
const newImports = ts.factory.createArrayLiteralExpression([
324+
...importsArray.elements.filter((item) => item !== httpClientTesting),
279325
]);
280-
} else {
281-
// We add the provider to the existing provider array
282-
newProviders = ts.factory.createArrayLiteralExpression([
283-
...providers.elements,
284-
provideHttpClient,
285-
provideHttpClientTesting,
326+
327+
const provideHttpClient = createCallExpression(PROVIDE_HTTP_CLIENT, [
328+
createCallExpression(WITH_INTERCEPTORS_FROM_DI),
286329
]);
287-
}
330+
const provideHttpClientTesting = createCallExpression(PROVIDE_HTTP_CLIENT_TESTING);
331+
332+
// Adding the new providers
333+
const providers = getProvidersFromLiteralExpr(configureTestingModuleArgs);
334+
335+
let newProviders: ts.ArrayLiteralExpression;
336+
if (!providers) {
337+
// No existing providers, we add a property to the literal
338+
newProviders = ts.factory.createArrayLiteralExpression([
339+
provideHttpClient,
340+
provideHttpClientTesting,
341+
]);
342+
} else {
343+
// We add the provider to the existing provider array
344+
newProviders = ts.factory.updateArrayLiteralExpression(
345+
providers,
346+
ts.factory.createNodeArray(
347+
[...providers.elements, provideHttpClient, provideHttpClientTesting],
348+
providers.elements.hasTrailingComma,
349+
),
350+
);
351+
}
288352

289-
// Replacing the existing decorator with the new one (with the new imports and providers)
290-
const newTestingModuleArgs = ts.factory.createObjectLiteralExpression([
291-
...configureTestingModuleArgs.properties.filter((p) => p.getText() === 'imports'),
292-
ts.factory.createPropertyAssignment('imports', newImports),
293-
ts.factory.createPropertyAssignment('providers', newProviders),
294-
]);
295-
changeTracker.replaceNode(configureTestingModuleArgs, newTestingModuleArgs);
353+
// Replacing the existing configuration with the new one (with the new imports and providers)
354+
const newTestingModuleArgs = updateTestBedConfiguration(
355+
configureTestingModuleArgs,
356+
newImports,
357+
newProviders,
358+
);
359+
changeTracker.replaceNode(configureTestingModuleArgs, newTestingModuleArgs);
360+
}
296361
}
297362

298363
function getImportsProp(literal: ts.ObjectLiteralExpression) {
@@ -387,3 +452,14 @@ function createCallExpression(functionName: string, args: ts.Expression[] = [])
387452
args,
388453
);
389454
}
455+
456+
function updateTestBedConfiguration(
457+
configureTestingModuleArgs: ts.ObjectLiteralExpression,
458+
newImports: ts.ArrayLiteralExpression,
459+
newProviders: ts.ArrayLiteralExpression,
460+
): ts.ObjectLiteralExpression {
461+
return ts.factory.updateObjectLiteralExpression(configureTestingModuleArgs, [
462+
ts.factory.createPropertyAssignment('imports', newImports),
463+
ts.factory.createPropertyAssignment('providers', newProviders),
464+
]);
465+
}

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

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ describe('Http providers migration', () => {
241241
);
242242
});
243243

244-
it('should handle a migration for acomponent ', async () => {
244+
it('should handle a migration for a component', async () => {
245245
writeFile(
246246
'/index.ts',
247247
`
@@ -264,6 +264,32 @@ describe('Http providers migration', () => {
264264
expect(content).toContain(`provideHttpClient(withInterceptorsFromDi(), withJsonpSupport())`);
265265
});
266266

267+
it('should handle a migration of HttpClientModule in a test', async () => {
268+
writeFile(
269+
'/index.ts',
270+
`
271+
import { HttpClientModule } from '@angular/common/http';
272+
273+
describe('MyComponent', () => {
274+
beforeEach(() =>
275+
TestBed.configureTestingModule({
276+
imports: [HttpClientModule]
277+
})
278+
);
279+
});
280+
`,
281+
);
282+
283+
await runMigration();
284+
285+
const content = tree.readContent('/index.ts');
286+
expect(content).not.toContain(`'@angular/common/http/testing'`);
287+
expect(content).toContain(`'@angular/common/http'`);
288+
expect(content).toMatch(/import.*provideHttpClient.*withInterceptorsFromDi.*from/);
289+
expect(content).not.toContain(`HttpClientModule`);
290+
expect(content).toContain(`provideHttpClient(withInterceptorsFromDi())`);
291+
});
292+
267293
it('should not migrate HttpClientModule from another package', async () => {
268294
writeFile(
269295
'/index.ts',
@@ -318,7 +344,9 @@ describe('Http providers migration', () => {
318344
await runMigration();
319345

320346
const content = tree.readContent('/index.ts');
321-
expect(content).toContain(`@angular/common/http/testing`);
347+
expect(content).toContain(`'@angular/common/http/testing'`);
348+
expect(content).toContain(`'@angular/common/http'`);
349+
expect(content).toMatch(/import.*provideHttpClient.*withInterceptorsFromDi.*from/);
322350
expect(content).not.toContain(`HttpClientTestingModule`);
323351
expect(content).toMatch(/import.*provideHttpClientTesting/);
324352
expect(content).toContain(
@@ -347,7 +375,7 @@ describe('Http providers migration', () => {
347375
expect(content).not.toContain('provideHttpClientTesting');
348376
});
349377

350-
it('shouldmigrate NgModule + TestBed.configureTestingModule in the same file', async () => {
378+
it('should migrate NgModule + TestBed.configureTestingModule in the same file', async () => {
351379
writeFile(
352380
'/index.ts',
353381
`

‎packages/core/schematics/utils/change_tracker.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,8 @@ export class ChangeTracker {
110110
* @param sourceFile File to which to add the import.
111111
* @param symbolName Symbol being imported.
112112
* @param moduleName Module from which the symbol is imported.
113+
* @param alias Alias to use for the import.
114+
* @param keepSymbolName Whether to keep the symbol name in the import.
113115
*/
114116
addImport(
115117
sourceFile: ts.SourceFile,

0 commit comments

Comments
 (0)