From 00076e90650547aaab08535108971a127642a63a Mon Sep 17 00:00:00 2001 From: Rainer Hahnekamp Date: Sat, 15 Jun 2024 21:18:56 +0200 Subject: [PATCH 1/2] fix(effects): fix bugs in migration script to v18 This commit fixes one major bug and some edge cases in the migration script. - Reset the changes array, containing the necessary changes, with every file. - Use `os.EOL` instead of `\n`. - Fix an issue when a file has multiple import declaration to `@ngrx/effects`. - Print out a warning, if `concatLatestFrom` is imported multiple times. --- .../migrations/18_0_0-beta/index.spec.ts | 91 ++++++++++++++++++- .../effects/migrations/18_0_0-beta/index.ts | 29 +++++- 2 files changed, 114 insertions(+), 6 deletions(-) diff --git a/modules/effects/migrations/18_0_0-beta/index.spec.ts b/modules/effects/migrations/18_0_0-beta/index.spec.ts index 0f54268f9f..52d7b404ed 100644 --- a/modules/effects/migrations/18_0_0-beta/index.spec.ts +++ b/modules/effects/migrations/18_0_0-beta/index.spec.ts @@ -5,6 +5,7 @@ import { import { createWorkspace } from '@ngrx/schematics-core/testing'; import { tags } from '@angular-devkit/core'; import * as path from 'path'; +import { LogEntry } from '@angular-devkit/core/src/logger'; describe('Effects Migration to 18.0.0-beta', () => { const collectionPath = path.join(__dirname, '../migration.json'); @@ -121,7 +122,95 @@ export class SomeEffects { }); }); - it('should add if they are missing', async () => { + it('should work with prior import from same namespace', async () => { + const input = tags.stripIndent` +import { Actions } from '@ngrx/effects'; +import { concatLatestFrom, createEffect, ofType } from '@ngrx/effects'; + +class SomeEffects {} + `; + const output = tags.stripIndent` +import { Actions } from '@ngrx/effects'; +import { createEffect, ofType } from '@ngrx/effects'; +import { concatLatestFrom } from '@ngrx/operators'; + +class SomeEffects {} + `; + await verifySchematic(input, output); + }); + + it('should operate on multiple files', async () => { + const inputMainOne = tags.stripIndent` +import { Actions, concatLatestFrom, createEffect, ofType } from '@ngrx/effects'; +import { tap } from 'rxjs/operators'; + +class SomeEffects {} +`; + + const outputMainOne = tags.stripIndent` +import { Actions, createEffect, ofType } from '@ngrx/effects'; +import { concatLatestFrom } from '@ngrx/operators'; +import { tap } from 'rxjs/operators'; + +class SomeEffects {} +`; + + const inputMainTwo = tags.stripIndent` +import { provideEffects } from '@ngrx/effects'; +import { Actions, concatLatestFrom, createEffect, ofType } from '@ngrx/effects'; + +class SomeEffects {} +`; + + const outputMainTwo = tags.stripIndent` +import { provideEffects } from '@ngrx/effects'; +import { Actions, createEffect, ofType } from '@ngrx/effects'; +import { concatLatestFrom } from '@ngrx/operators'; + +class SomeEffects {} +`; + appTree.create('mainOne.ts', inputMainOne); + appTree.create('mainTwo.ts', inputMainTwo); + + const tree = await schematicRunner.runSchematic( + `ngrx-effects-migration-18-beta`, + {}, + appTree + ); + + const actualMainOne = tree.readContent('mainOne.ts'); + const actualMainTwo = tree.readContent('mainTwo.ts'); + + expect(actualMainOne).toBe(outputMainOne); + expect(actualMainTwo).toBe(outputMainTwo); + }); + + it('should report a warning on multiple imports of concatLatestFrom', async () => { + const input = tags.stripIndent` +import { concatLatestFrom } from '@ngrx/effects'; +import { concatLatestFrom, createEffect, ofType } from '@ngrx/effects'; + +class SomeEffects {} + `; + + appTree.create('main.ts', input); + const logEntries: LogEntry[] = []; + schematicRunner.logger.subscribe((logEntry) => logEntries.push(logEntry)); + await schematicRunner.runSchematic( + `ngrx-effects-migration-18-beta`, + {}, + appTree + ); + + expect(logEntries).toHaveLength(1); + expect(logEntries[0]).toMatchObject({ + message: + '[@ngrx/effects] Skipping because of multiple `concatLatestFrom` imports', + level: 'warn', + }); + }); + + it('should add @ngrx/operators if they are missing', async () => { const originalPackageJson = JSON.parse( appTree.readContent('/package.json') ); diff --git a/modules/effects/migrations/18_0_0-beta/index.ts b/modules/effects/migrations/18_0_0-beta/index.ts index e47809e1b5..8aca2744db 100644 --- a/modules/effects/migrations/18_0_0-beta/index.ts +++ b/modules/effects/migrations/18_0_0-beta/index.ts @@ -14,29 +14,47 @@ import { visitTSSourceFiles, } from '../../schematics-core'; import { createRemoveChange } from '../../schematics-core/utility/change'; +import * as os from 'node:os'; export function migrateConcatLatestFromImport(): Rule { return (tree: Tree, ctx: SchematicContext) => { - const changes: Change[] = []; addPackageToPackageJson(tree, 'dependencies', '@ngrx/operators', '^18.0.0'); visitTSSourceFiles(tree, (sourceFile) => { const importDeclarations = new Array(); + getImportDeclarations(sourceFile, importDeclarations); - const effectsImportsAndDeclaration = importDeclarations + const effectsImportsAndDeclarations = importDeclarations .map((effectsImportDeclaration) => { const effectsImports = getEffectsNamedBinding( effectsImportDeclaration ); if (effectsImports) { - return { effectsImports, effectsImportDeclaration }; + if ( + effectsImports.elements.some( + (element) => element.name.getText() === 'concatLatestFrom' + ) + ) { + return { effectsImports, effectsImportDeclaration }; + } + return undefined; } else { return undefined; } }) - .find(Boolean); + .filter(Boolean); + + if (effectsImportsAndDeclarations.length === 0) { + return; + } else if (effectsImportsAndDeclarations.length > 1) { + ctx.logger.warn( + '[@ngrx/effects] Skipping because of multiple `concatLatestFrom` imports' + ); + return; + } + const [effectsImportsAndDeclaration] = effectsImportsAndDeclarations; if (!effectsImportsAndDeclaration) { return; } @@ -53,6 +71,7 @@ export function migrateConcatLatestFromImport(): Rule { .map((element) => element.name.getText()) .join(', '); + const changes: Change[] = []; // Remove `concatLatestFrom` from @ngrx/effects and leave the other imports if (otherEffectsImports) { changes.push( @@ -107,7 +126,7 @@ export function migrateConcatLatestFromImport(): Rule { new InsertChange( sourceFile.fileName, effectsImportDeclaration.getEnd() + 1, - `${newOperatorsImport}\n` + `${newOperatorsImport}${os.EOL}` ) ); } From e4d41555f830b4e13e230724512455112efbd5b7 Mon Sep 17 00:00:00 2001 From: Rainer Hahnekamp Date: Mon, 17 Jun 2024 17:16:09 +0200 Subject: [PATCH 2/2] fix(effects): applying changes due to code review --- modules/effects/migrations/18_0_0-beta/index.spec.ts | 4 ++-- modules/effects/migrations/18_0_0-beta/index.ts | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/effects/migrations/18_0_0-beta/index.spec.ts b/modules/effects/migrations/18_0_0-beta/index.spec.ts index 52d7b404ed..a4e3ef93c0 100644 --- a/modules/effects/migrations/18_0_0-beta/index.spec.ts +++ b/modules/effects/migrations/18_0_0-beta/index.spec.ts @@ -185,7 +185,7 @@ class SomeEffects {} expect(actualMainTwo).toBe(outputMainTwo); }); - it('should report a warning on multiple imports of concatLatestFrom', async () => { + it('should report an info on multiple imports of concatLatestFrom', async () => { const input = tags.stripIndent` import { concatLatestFrom } from '@ngrx/effects'; import { concatLatestFrom, createEffect, ofType } from '@ngrx/effects'; @@ -206,7 +206,7 @@ class SomeEffects {} expect(logEntries[0]).toMatchObject({ message: '[@ngrx/effects] Skipping because of multiple `concatLatestFrom` imports', - level: 'warn', + level: 'info', }); }); diff --git a/modules/effects/migrations/18_0_0-beta/index.ts b/modules/effects/migrations/18_0_0-beta/index.ts index 8aca2744db..6448d9e0a4 100644 --- a/modules/effects/migrations/18_0_0-beta/index.ts +++ b/modules/effects/migrations/18_0_0-beta/index.ts @@ -14,7 +14,6 @@ import { visitTSSourceFiles, } from '../../schematics-core'; import { createRemoveChange } from '../../schematics-core/utility/change'; -import * as os from 'node:os'; export function migrateConcatLatestFromImport(): Rule { return (tree: Tree, ctx: SchematicContext) => { @@ -48,7 +47,7 @@ export function migrateConcatLatestFromImport(): Rule { if (effectsImportsAndDeclarations.length === 0) { return; } else if (effectsImportsAndDeclarations.length > 1) { - ctx.logger.warn( + ctx.logger.info( '[@ngrx/effects] Skipping because of multiple `concatLatestFrom` imports' ); return; @@ -126,7 +125,7 @@ export function migrateConcatLatestFromImport(): Rule { new InsertChange( sourceFile.fileName, effectsImportDeclaration.getEnd() + 1, - `${newOperatorsImport}${os.EOL}` + `${newOperatorsImport}\n` // not os-independent for snapshot tests ) ); }