Skip to content

Commit

Permalink
fix(migrations): handle more cases in HttpClientModule migration
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@cexbrayat
cexbrayat committed May 4, 2024
1 parent 1872fcd commit 3937b3b
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 62 deletions.
53 changes: 42 additions & 11 deletions packages/core/schematics/migrations/http-providers/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
```
Expand All @@ -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())]
})
})
}
})
```
172 changes: 124 additions & 48 deletions packages/core/schematics/migrations/http-providers/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,13 @@ export function migrateFile(
});
}

migrateTestingModuleImports(node, commonHttpTestingIdentifiers, addedImports, changeTracker);
migrateTestingModuleImports(
node,
commonHttpIdentifiers,
commonHttpTestingIdentifiers,
addedImports,
changeTracker,
);
});

// Imports are for the whole file
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand All @@ -219,6 +236,7 @@ function migrateDecorator(

function migrateTestingModuleImports(
node: ts.Node,
commonHttpIdentifiers: Set<string>,
commonHttpTestingIdentifiers: Set<string>,
addedImports: Map<string, Set<string>>,
changeTracker: ChangeTracker,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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),
]);
}
34 changes: 31 additions & 3 deletions packages/core/schematics/test/http_providers_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
`
Expand All @@ -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',
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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',
`
Expand Down
2 changes: 2 additions & 0 deletions packages/core/schematics/utils/change_tracker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 3937b3b

Please sign in to comment.