fix(migrations): handle more cases in HttpClientModule migration (angular#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 angular#55640

Author: cexbrayat

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

@@ -13,7 +13,7 @@ This migration updates any `@NgModule`, `@Component`, `@Directive` that imports
 import { HttpClientModule, HttpClientJsonpModule, HttpClientXsrfModule } from '@angular/common/http';
 
 @NgModule({
-    imports: [CommonModule, HttpClientModule,HttpClientJsonpModule, HttpClientXsrfModule)],
+    imports: [CommonModule, HttpClientModule, HttpClientJsonpModule, HttpClientXsrfModule]
 })
 export class AppModule {}
 ```
@@ -33,30 +33,61 @@ export class AppModule {}
 
 #### Before 
 
+```ts
+import { HttpClientTestingModule } from '@angular/common/http/testing';
+
+describe('some test', () => {
+
+    it('...', () => {
+      TestBed.configureTestingModule({
+        imports: [HttpClientTestingModule]
+      })
+    })
+})
 ```
-import { HttpClientTestingModule } from '@not-angular/common/http/testing';
 
-describe('some test') {
+#### After
+
+```ts
+import { provideHttpClientTesting } from '@angular/common/http/testing';
+import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http';
+
+describe('some test', () => {
 
     it('...', () => {
       TestBed.configureTestingModule({
-        imports: [HttpClientTestingModule],
-      });
+        providers: [provideHttpClient(withInterceptorsFromDi()), provideHttpClientTesting()]
+      })
     })
-}
+})
 ```
 
 #### Before
 
+```ts
+import { HttpClientTesting } from '@angular/common/http';
+
+describe('some test', () => {
+
+    it('...', () => {
+      TestBed.configureTestingModule({
+        imports: [HttpClientTesting],
+      })
+    })
+});
 ```
-import { provideHttpClientTesting } from '@not-angular/common/http/testing';
 
-describe('some test') {
+#### After
+
+```ts
+import { provideHttpClient, withInterceptorsFromDi } from '@angular/common/http';
+
+describe('some test', () => {
 
     it('...', () => {
       TestBed.configureTestingModule({
-        providers: [provideHttpClientTesting()],
-      });
+        providers: [provideHttpClient(withInterceptorsFromDi())]
+      })
     })
-}
+})
 ```

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

@@ -67,7 +67,13 @@ export function migrateFile(
       });
     }
 
-    migrateTestingModuleImports(node, commonHttpTestingIdentifiers, addedImports, changeTracker);
+    migrateTestingModuleImports(
+      node,
+      commonHttpIdentifiers,
+      commonHttpTestingIdentifiers,
+      addedImports,
+      changeTracker,
+    );
   });
 
   // Imports are for the whole file
@@ -90,6 +96,13 @@ export function migrateFile(
     ]);
     changeTracker.replaceNode(commonHttpImports, newImports);
   }
+  // If there are no imports for common/http, and we need to add some
+  else if (addedImports.get(COMMON_HTTP)?.size) {
+    // Then we add a new import statement for common/http
+    addedImports.get(COMMON_HTTP)?.forEach((entry) => {
+      changeTracker.addImport(sourceFile, entry, COMMON_HTTP);
+    });
+  }
 
   // Remove the HttpModules imports from common/http/testing
   const commonHttpTestingImports = getNamedImports(sourceFile, COMMON_HTTP_TESTING);
@@ -156,22 +169,23 @@ function migrateDecorator(
   const addedProviders = new Set<ts.CallExpression>();
 
   // Handle the different imported Http modules
+  const commonHttpAddedImports = addedImports.get(COMMON_HTTP);
   if (importedModules.client) {
-    addedImports.get(COMMON_HTTP)?.add(WITH_INTERCEPTORS_FROM_DI);
+    commonHttpAddedImports?.add(WITH_INTERCEPTORS_FROM_DI);
     addedProviders.add(createCallExpression(WITH_INTERCEPTORS_FROM_DI));
   }
   if (importedModules.clientJsonp) {
-    addedImports.get(COMMON_HTTP)?.add(WITH_JSONP_SUPPORT);
+    commonHttpAddedImports?.add(WITH_JSONP_SUPPORT);
     addedProviders.add(createCallExpression(WITH_JSONP_SUPPORT));
   }
   if (importedModules.xsrf) {
     // HttpClientXsrfModule is the only module with Class methods.
     // They correspond to different provider functions
     if (importedModules.xsrfOptions === 'disable') {
-      addedImports.get(COMMON_HTTP)?.add(WITH_NOXSRF_PROTECTION);
+      commonHttpAddedImports?.add(WITH_NOXSRF_PROTECTION);
       addedProviders.add(createCallExpression(WITH_NOXSRF_PROTECTION));
     } else {
-      addedImports.get(COMMON_HTTP)?.add(WITH_XSRF_CONFIGURATION);
+      commonHttpAddedImports?.add(WITH_XSRF_CONFIGURATION);
       addedProviders.add(
         createCallExpression(
           WITH_XSRF_CONFIGURATION,
@@ -192,20 +206,23 @@ function migrateDecorator(
   ]);
 
   // Adding the new providers
-  addedImports.get(COMMON_HTTP)?.add(PROVIDE_HTTP_CLIENT);
+  commonHttpAddedImports?.add(PROVIDE_HTTP_CLIENT);
   const providers = getProvidersFromLiteralExpr(metadata);
   const provideHttpExpr = createCallExpression(PROVIDE_HTTP_CLIENT, [...addedProviders]);
 
   let newProviders: ts.ArrayLiteralExpression;
   if (!providers) {
-    // No existing providers, we add an property to the literal
+    // No existing providers, we add a property to the literal
     newProviders = ts.factory.createArrayLiteralExpression([provideHttpExpr]);
   } else {
     // We add the provider to the existing provider array
-    newProviders = ts.factory.createArrayLiteralExpression([
-      ...providers.elements,
-      provideHttpExpr,
-    ]);
+    newProviders = ts.factory.updateArrayLiteralExpression(
+      providers,
+      ts.factory.createNodeArray(
+        [...providers.elements, provideHttpExpr],
+        providers.elements.hasTrailingComma,
+      ),
+    );
   }
 
   // Replacing the existing decorator with the new one (with the new imports and providers)
@@ -219,6 +236,7 @@ function migrateDecorator(
 
 function migrateTestingModuleImports(
   node: ts.Node,
+  commonHttpIdentifiers: Set<string>,
   commonHttpTestingIdentifiers: Set<string>,
   addedImports: Map<string, Set<string>>,
   changeTracker: ChangeTracker,
@@ -248,51 +266,98 @@ function migrateTestingModuleImports(
     return;
   }
 
-  // Does the imports array contain the HttpClientTestingModule?
-  const httpClientTesting = importsArray.elements.find(
-    (elt) => elt.getText() === HTTP_CLIENT_TESTING_MODULE,
-  );
-  if (!httpClientTesting || !commonHttpTestingIdentifiers.has(HTTP_CLIENT_TESTING_MODULE)) {
-    return;
-  }
+  const commonHttpAddedImports = addedImports.get(COMMON_HTTP);
 
-  addedImports.get(COMMON_HTTP_TESTING)?.add(PROVIDE_HTTP_CLIENT_TESTING);
+  // Does the imports array contain the HttpClientModule?
+  const httpClient = importsArray.elements.find((elt) => elt.getText() === HTTP_CLIENT_MODULE);
+  if (httpClient && commonHttpIdentifiers.has(HTTP_CLIENT_MODULE)) {
+    // We add the imports for provideHttpClient(withInterceptorsFromDi())
+    commonHttpAddedImports?.add(PROVIDE_HTTP_CLIENT);
+    commonHttpAddedImports?.add(WITH_INTERCEPTORS_FROM_DI);
 
-  const newImports = ts.factory.createArrayLiteralExpression([
-    ...importsArray.elements.filter((item) => item !== httpClientTesting),
-  ]);
+    const newImports = ts.factory.createArrayLiteralExpression([
+      ...importsArray.elements.filter((item) => item !== httpClient),
+    ]);
 
-  const provideHttpClient = createCallExpression(PROVIDE_HTTP_CLIENT, [
-    createCallExpression(WITH_INTERCEPTORS_FROM_DI),
-  ]);
-  const provideHttpClientTesting = createCallExpression(PROVIDE_HTTP_CLIENT_TESTING);
+    const provideHttpClient = createCallExpression(PROVIDE_HTTP_CLIENT, [
+      createCallExpression(WITH_INTERCEPTORS_FROM_DI),
+    ]);
 
-  // Adding the new providers
-  const providers = getProvidersFromLiteralExpr(configureTestingModuleArgs);
+    // Adding the new provider
+    const providers = getProvidersFromLiteralExpr(configureTestingModuleArgs);
 
-  let newProviders: ts.ArrayLiteralExpression;
-  if (!providers) {
-    // No existing providers, we add an property to the literal
-    newProviders = ts.factory.createArrayLiteralExpression([
-      provideHttpClient,
-      provideHttpClientTesting,
+    let newProviders: ts.ArrayLiteralExpression;
+    if (!providers) {
+      // No existing providers, we add a property to the literal
+      newProviders = ts.factory.createArrayLiteralExpression([provideHttpClient]);
+    } else {
+      // We add the provider to the existing provider array
+      newProviders = ts.factory.updateArrayLiteralExpression(
+        providers,
+        ts.factory.createNodeArray(
+          [...providers.elements, provideHttpClient],
+          providers.elements.hasTrailingComma,
+        ),
+      );
+    }
+
+    // Replacing the existing configuration with the new one (with the new imports and providers)
+    const newTestingModuleArgs = updateTestBedConfiguration(
+      configureTestingModuleArgs,
+      newImports,
+      newProviders,
+    );
+    changeTracker.replaceNode(configureTestingModuleArgs, newTestingModuleArgs);
+  }
+
+  // Does the imports array contain the HttpClientTestingModule?
+  const httpClientTesting = importsArray.elements.find(
+    (elt) => elt.getText() === HTTP_CLIENT_TESTING_MODULE,
+  );
+  if (httpClientTesting && commonHttpTestingIdentifiers.has(HTTP_CLIENT_TESTING_MODULE)) {
+    // We add the imports for provideHttpClient(withInterceptorsFromDi()) and provideHttpClientTesting()
+    commonHttpAddedImports?.add(PROVIDE_HTTP_CLIENT);
+    commonHttpAddedImports?.add(WITH_INTERCEPTORS_FROM_DI);
+    addedImports.get(COMMON_HTTP_TESTING)?.add(PROVIDE_HTTP_CLIENT_TESTING);
+
+    const newImports = ts.factory.createArrayLiteralExpression([
+      ...importsArray.elements.filter((item) => item !== httpClientTesting),
     ]);
-  } else {
-    // We add the provider to the existing provider array
-    newProviders = ts.factory.createArrayLiteralExpression([
-      ...providers.elements,
-      provideHttpClient,
-      provideHttpClientTesting,
+
+    const provideHttpClient = createCallExpression(PROVIDE_HTTP_CLIENT, [
+      createCallExpression(WITH_INTERCEPTORS_FROM_DI),
     ]);
-  }
+    const provideHttpClientTesting = createCallExpression(PROVIDE_HTTP_CLIENT_TESTING);
+
+    // Adding the new providers
+    const providers = getProvidersFromLiteralExpr(configureTestingModuleArgs);
+
+    let newProviders: ts.ArrayLiteralExpression;
+    if (!providers) {
+      // No existing providers, we add a property to the literal
+      newProviders = ts.factory.createArrayLiteralExpression([
+        provideHttpClient,
+        provideHttpClientTesting,
+      ]);
+    } else {
+      // We add the provider to the existing provider array
+      newProviders = ts.factory.updateArrayLiteralExpression(
+        providers,
+        ts.factory.createNodeArray(
+          [...providers.elements, provideHttpClient, provideHttpClientTesting],
+          providers.elements.hasTrailingComma,
+        ),
+      );
+    }
 
-  // Replacing the existing decorator with the new one (with the new imports and providers)
-  const newTestingModuleArgs = ts.factory.createObjectLiteralExpression([
-    ...configureTestingModuleArgs.properties.filter((p) => p.getText() === 'imports'),
-    ts.factory.createPropertyAssignment('imports', newImports),
-    ts.factory.createPropertyAssignment('providers', newProviders),
-  ]);
-  changeTracker.replaceNode(configureTestingModuleArgs, newTestingModuleArgs);
+    // Replacing the existing configuration with the new one (with the new imports and providers)
+    const newTestingModuleArgs = updateTestBedConfiguration(
+      configureTestingModuleArgs,
+      newImports,
+      newProviders,
+    );
+    changeTracker.replaceNode(configureTestingModuleArgs, newTestingModuleArgs);
+  }
 }
 
 function getImportsProp(literal: ts.ObjectLiteralExpression) {
@@ -387,3 +452,14 @@ function createCallExpression(functionName: string, args: ts.Expression[] = [])
     args,
   );
 }
+
+function updateTestBedConfiguration(
+  configureTestingModuleArgs: ts.ObjectLiteralExpression,
+  newImports: ts.ArrayLiteralExpression,
+  newProviders: ts.ArrayLiteralExpression,
+): ts.ObjectLiteralExpression {
+  return ts.factory.updateObjectLiteralExpression(configureTestingModuleArgs, [
+    ts.factory.createPropertyAssignment('imports', newImports),
+    ts.factory.createPropertyAssignment('providers', newProviders),
+  ]);
+}

packages/core/schematics/test/http_providers_spec.ts

@@ -241,7 +241,7 @@ describe('Http providers migration', () => {
     );
   });
 
-  it('should handle a migration for acomponent ', async () => {
+  it('should handle a migration for a component', async () => {
     writeFile(
       '/index.ts',
       `
@@ -264,6 +264,32 @@ describe('Http providers migration', () => {
     expect(content).toContain(`provideHttpClient(withInterceptorsFromDi(), withJsonpSupport())`);
   });
 
+  it('should handle a migration of HttpClientModule in a test', async () => {
+    writeFile(
+      '/index.ts',
+      `
+          import { HttpClientModule } from '@angular/common/http';
+
+          describe('MyComponent', () => {
+            beforeEach(() =>
+              TestBed.configureTestingModule({
+                imports: [HttpClientModule]
+              })
+            );
+          });
+    `,
+    );
+
+    await runMigration();
+
+    const content = tree.readContent('/index.ts');
+    expect(content).not.toContain(`'@angular/common/http/testing'`);
+    expect(content).toContain(`'@angular/common/http'`);
+    expect(content).toMatch(/import.*provideHttpClient.*withInterceptorsFromDi.*from/);
+    expect(content).not.toContain(`HttpClientModule`);
+    expect(content).toContain(`provideHttpClient(withInterceptorsFromDi())`);
+  });
+
   it('should not migrate HttpClientModule from another package', async () => {
     writeFile(
       '/index.ts',
@@ -318,7 +344,9 @@ describe('Http providers migration', () => {
     await runMigration();
 
     const content = tree.readContent('/index.ts');
-    expect(content).toContain(`@angular/common/http/testing`);
+    expect(content).toContain(`'@angular/common/http/testing'`);
+    expect(content).toContain(`'@angular/common/http'`);
+    expect(content).toMatch(/import.*provideHttpClient.*withInterceptorsFromDi.*from/);
     expect(content).not.toContain(`HttpClientTestingModule`);
     expect(content).toMatch(/import.*provideHttpClientTesting/);
     expect(content).toContain(
@@ -347,7 +375,7 @@ describe('Http providers migration', () => {
     expect(content).not.toContain('provideHttpClientTesting');
   });
 
-  it('shouldmigrate NgModule + TestBed.configureTestingModule in the same file', async () => {
+  it('should migrate NgModule + TestBed.configureTestingModule in the same file', async () => {
     writeFile(
       '/index.ts',
       `

packages/core/schematics/utils/change_tracker.ts

@@ -110,6 +110,8 @@ export class ChangeTracker {
    * @param sourceFile File to which to add the import.
    * @param symbolName Symbol being imported.
    * @param moduleName Module from which the symbol is imported.
+   * @param alias Alias to use for the import.
+   * @param keepSymbolName Whether to keep the symbol name in the import.
    */
   addImport(
     sourceFile: ts.SourceFile,