diff --git a/modules/data/schematics-core/utility/ast-utils.ts b/modules/data/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/data/schematics-core/utility/ast-utils.ts +++ b/modules/data/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/data/schematics-core/utility/change.ts b/modules/data/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/data/schematics-core/utility/change.ts +++ b/modules/data/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/effects/schematics-core/utility/ast-utils.ts b/modules/effects/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/effects/schematics-core/utility/ast-utils.ts +++ b/modules/effects/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/effects/schematics-core/utility/change.ts b/modules/effects/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/effects/schematics-core/utility/change.ts +++ b/modules/effects/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/entity/schematics-core/utility/ast-utils.ts b/modules/entity/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/entity/schematics-core/utility/ast-utils.ts +++ b/modules/entity/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/entity/schematics-core/utility/change.ts b/modules/entity/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/entity/schematics-core/utility/change.ts +++ b/modules/entity/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/router-store/schematics-core/utility/ast-utils.ts b/modules/router-store/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/router-store/schematics-core/utility/ast-utils.ts +++ b/modules/router-store/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/router-store/schematics-core/utility/change.ts b/modules/router-store/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/router-store/schematics-core/utility/change.ts +++ b/modules/router-store/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/schematics-core/utility/ast-utils.ts b/modules/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/schematics-core/utility/ast-utils.ts +++ b/modules/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/schematics-core/utility/change.ts b/modules/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/schematics-core/utility/change.ts +++ b/modules/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/schematics/BUILD b/modules/schematics/BUILD index ea6795c34a..77112ca118 100644 --- a/modules/schematics/BUILD +++ b/modules/schematics/BUILD @@ -63,6 +63,7 @@ ts_test_library( deps = [ ":schematics", "//modules/schematics-core", + "@npm//@angular-devkit/core", "@npm//@angular-devkit/schematics", "@npm//@schematics/angular", ], diff --git a/modules/schematics/collection.json b/modules/schematics/collection.json index fd19a4c03d..d270e72dd3 100644 --- a/modules/schematics/collection.json +++ b/modules/schematics/collection.json @@ -22,6 +22,13 @@ "description": "Add side effect class" }, + "create-effect-migration": { + "aliases": ["cefm"], + "factory": "./src/create-effect-migration", + "schema": "./src/create-effect-migration/schema.json", + "description": "Migrated usages of @Effect() to createEffect()" + }, + "entity": { "aliases": ["en"], "factory": "./src/entity", diff --git a/modules/schematics/schematics-core/utility/ast-utils.ts b/modules/schematics/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/schematics/schematics-core/utility/ast-utils.ts +++ b/modules/schematics/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/schematics/schematics-core/utility/change.ts b/modules/schematics/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/schematics/schematics-core/utility/change.ts +++ b/modules/schematics/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/schematics/src/create-effect-migration/index.spec.ts b/modules/schematics/src/create-effect-migration/index.spec.ts new file mode 100644 index 0000000000..6d587bb2ce --- /dev/null +++ b/modules/schematics/src/create-effect-migration/index.spec.ts @@ -0,0 +1,227 @@ +import { tags } from '@angular-devkit/core'; +import { + SchematicTestRunner, + UnitTestTree, +} from '@angular-devkit/schematics/testing'; +import * as path from 'path'; +import { createWorkspace } from '../../../schematics-core/testing'; + +describe('Creator migration', async () => { + const schematicRunner = new SchematicTestRunner( + '@ngrx/schematics', + path.join(__dirname, '../../collection.json') + ); + + let appTree: UnitTestTree; + + beforeEach(async () => { + appTree = await createWorkspace(schematicRunner, appTree); + }); + + it('should use createEffect for non-dispatching effects', async () => { + const input = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + @Effect() + foo$ = this.actions$.pipe( + ofType(AuthActions.login), + tap(action => console.log(action)) + ); + } + `; + + const output = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + ** + foo$ = createEffect(() => this.actions$.pipe( + ofType(AuthActions.login), + tap(action => console.log(action)) + )); + } + `; + + await runTest(input, output); + }); + + it('should use createEffect for non-dispatching effects', async () => { + const input = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + @Effect({ dispatch: false }) + bar$ = this.actions$.pipe( + ofType(AuthActions.login, AuthActions.logout), + tap(action => console.log(action)) + ); + } + `; + + const output = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + ** + bar$ = createEffect(() => this.actions$.pipe( + ofType(AuthActions.login, AuthActions.logout), + tap(action => console.log(action)) + ), { dispatch: false }); + } + `; + + await runTest(input, output); + }); + + it('should use createEffect for effects as functions', async () => { + const input = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + @Effect() + baz$ = ({ debounce = 300, scheduler = asyncScheduler } = {}) => this.actions$.pipe( + ofType(login), + tap(action => console.log(action)) + ); + } + `; + + const output = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + ** + baz$ = createEffect(() => ({ debounce = 300, scheduler = asyncScheduler } = {}) => this.actions$.pipe( + ofType(login), + tap(action => console.log(action)) + )); + } + `; + + await runTest(input, output); + }); + + it('should stay off of other decorators', async () => { + const input = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + @Effect() + @Log() + login$ = this.actions$.pipe( + ofType('LOGIN'), + map(() => ({ type: 'LOGGED_IN' })) + ); + @Log() + @Effect() + logout$ = this.actions$.pipe( + ofType('LOGOUT'), + map(() => ({ type: 'LOGGED_OUT' })) + ); + } + `; + + const output = tags.stripIndent` + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + ** + @Log() + login$ = createEffect(() => this.actions$.pipe( + ofType('LOGIN'), + map(() => ({ type: 'LOGGED_IN' })) + )); + @Log() + ** + logout$ = createEffect(() => this.actions$.pipe( + ofType('LOGOUT'), + map(() => ({ type: 'LOGGED_OUT' })) + )); + } + `; + + await runTest(input, output); + }); + + it('should import createEffect', async () => { + const input = tags.stripIndent` + import { Actions, ofType, Effect } from '@ngrx/effects'; + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + } + `; + + const output = tags.stripIndent` + import { Actions, ofType, createEffect } from '@ngrx/effects'; + @Injectable() + export class SomeEffectsClass { + constructor(private actions$: Actions) {} + } + `; + + await runTest(input, output); + }); + + it('should not import createEffect if already imported', async () => { + const input = tags.stripIndent` + import { Actions, Effect, createEffect, ofType } from '@ngrx/effects'; + @Injectable() + export class SomeEffectsClass { + @Effect() + logout$ = this.actions$.pipe( + ofType('LOGOUT'), + map(() => ({ type: 'LOGGED_OUT' })) + ); + constructor(private actions$: Actions) {} + } + `; + + const output = tags.stripIndent` + import { Actions, createEffect, ofType } from '@ngrx/effects'; + @Injectable() + export class SomeEffectsClass { + ** + logout$ = createEffect(() => this.actions$.pipe( + ofType('LOGOUT'), + map(() => ({ type: 'LOGGED_OUT' })) + )); + constructor(private actions$: Actions) {} + } + `; + + await runTest(input, output); + }); + + it('should not run the schematic if the createEffect syntax is already used', async () => { + const input = tags.stripIndent` + import { Actions, createEffect, ofType } from '@ngrx/effects'; + @Injectable() + export class SomeEffectsClass { + logout$ = createEffect(() => this.actions$.pipe( + ofType('LOGOUT'), + map(() => ({ type: 'LOGGED_OUT' })) + )); + constructor(private actions$: Actions) {} + } + `; + + await runTest(input, input); + }); + + async function runTest(input: string, expected: string) { + const options = {}; + const effectPath = '/some.effects.ts'; + appTree.create(effectPath, input); + + const tree = await schematicRunner + .runSchematicAsync('create-effect-migration', options, appTree) + .toPromise(); + + const actual = tree.readContent(effectPath); + + // ** for indentation because empty lines are formatted on auto-save + expect(actual).toBe(expected.replace(/\*\*/g, ' ')); + } +}); diff --git a/modules/schematics/src/create-effect-migration/index.ts b/modules/schematics/src/create-effect-migration/index.ts new file mode 100644 index 0000000000..4b394cd5b3 --- /dev/null +++ b/modules/schematics/src/create-effect-migration/index.ts @@ -0,0 +1,128 @@ +import * as ts from 'typescript'; +import { Path } from '@angular-devkit/core'; +import { Tree, Rule, chain } from '@angular-devkit/schematics'; +import { + InsertChange, + RemoveChange, + replaceImport, + commitChanges, +} from '@ngrx/schematics/schematics-core'; + +export function migrateToCreators(): Rule { + return (host: Tree) => + host.visit(path => { + if (!path.endsWith('.ts')) { + return; + } + + const sourceFile = ts.createSourceFile( + path, + host.read(path)!.toString(), + ts.ScriptTarget.Latest + ); + + if (sourceFile.isDeclarationFile) { + return; + } + + const effectsPerClass = sourceFile.statements + .filter(ts.isClassDeclaration) + .map(clas => + clas.members + .filter(ts.isPropertyDeclaration) + .filter( + property => + property.decorators && + property.decorators.some(isEffectDecorator) + ) + ); + + const effects = effectsPerClass.reduce( + (acc, effects) => acc.concat(effects), + [] + ); + + const createEffectsChanges = replaceEffectDecorators(host, path, effects); + const importChanges = replaceImport( + sourceFile, + path, + '@ngrx/effects', + 'Effect', + 'createEffect' + ); + + return commitChanges(host, sourceFile.fileName, [ + ...importChanges, + ...createEffectsChanges, + ]); + }); +} + +function replaceEffectDecorators( + host: Tree, + path: Path, + effects: ts.PropertyDeclaration[] +) { + const inserts = effects + .filter(effect => !!effect.initializer) + .map(effect => { + const decorator = (effect.decorators || []).find(isEffectDecorator)!; + const effectArguments = getDispatchProperties(host, path, decorator); + const end = effectArguments ? `, ${effectArguments})` : ')'; + + return [ + new InsertChange(path, effect.initializer!.pos, ' createEffect(() =>'), + new InsertChange(path, effect.initializer!.end, end), + ]; + }) + .reduce((acc, inserts) => acc.concat(inserts), []); + + const removes = effects + .map(effect => effect.decorators) + .filter(decorators => decorators) + .map(decorators => { + const effectDecorators = decorators!.filter(isEffectDecorator); + return effectDecorators.map(decorator => { + return new RemoveChange( + path, + decorator.expression.pos - 1, // also get the @ sign + decorator.expression.end + ); + }); + }) + .reduce((acc, removes) => acc.concat(removes), []); + + return [...inserts, ...removes]; +} + +function isEffectDecorator(decorator: ts.Decorator) { + return ( + ts.isCallExpression(decorator.expression) && + ts.isIdentifier(decorator.expression.expression) && + decorator.expression.expression.text === 'Effect' + ); +} + +function getDispatchProperties( + host: Tree, + path: Path, + decorator: ts.Decorator +) { + if (!decorator.expression || !ts.isCallExpression(decorator.expression)) { + return ''; + } + + // just copy the effect properties + const content = host.read(path)!.toString('utf8'); + const args = content + .substring( + decorator.expression.arguments.pos, + decorator.expression.arguments.end + ) + .trim(); + return args; +} + +export default function(): Rule { + return chain([migrateToCreators()]); +} diff --git a/modules/schematics/src/create-effect-migration/schema.json b/modules/schematics/src/create-effect-migration/schema.json new file mode 100644 index 0000000000..fdcfffba1a --- /dev/null +++ b/modules/schematics/src/create-effect-migration/schema.json @@ -0,0 +1,8 @@ +{ + "$schema": "http://json-schema.org/schema", + "id": "SchematicsNgRxCreateEffectMigration", + "title": "NgRx Effect Migration from @Effect to createEffect", + "type": "object", + "properties": {}, + "required": [] +} diff --git a/modules/schematics/src/create-effect-migration/schema.ts b/modules/schematics/src/create-effect-migration/schema.ts new file mode 100644 index 0000000000..e53f1202a2 --- /dev/null +++ b/modules/schematics/src/create-effect-migration/schema.ts @@ -0,0 +1 @@ +export interface Schema {} diff --git a/modules/store-devtools/schematics-core/utility/ast-utils.ts b/modules/store-devtools/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/store-devtools/schematics-core/utility/ast-utils.ts +++ b/modules/store-devtools/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/store-devtools/schematics-core/utility/change.ts b/modules/store-devtools/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/store-devtools/schematics-core/utility/change.ts +++ b/modules/store-devtools/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string, diff --git a/modules/store/schematics-core/utility/ast-utils.ts b/modules/store/schematics-core/utility/ast-utils.ts index 0404340135..8471c23228 100644 --- a/modules/store/schematics-core/utility/ast-utils.ts +++ b/modules/store/schematics-core/utility/ast-utils.ts @@ -13,6 +13,8 @@ import { NoopChange, createReplaceChange, ReplaceChange, + RemoveChange, + createRemoveChange, } from './change'; import { Path } from '@angular-devkit/core'; @@ -650,7 +652,7 @@ export function replaceImport( importFrom: string, importAsIs: string, importToBe: string -): ReplaceChange[] { +): (ReplaceChange | RemoveChange)[] { const imports = sourceFile.statements .filter(ts.isImportDeclaration) .filter( @@ -663,32 +665,67 @@ export function replaceImport( return []; } - const changes = imports - .map(p => (p.importClause!.namedBindings! as ts.NamedImports).elements) - .reduce((imports, curr) => imports.concat(curr), [] as ts.ImportSpecifier[]) - .map(specifier => { - if (!ts.isImportSpecifier(specifier)) { - return { hit: false }; + const importText = (specifier: ts.ImportSpecifier) => { + if (specifier.name.text) { + return specifier.name.text; + } + + // if import is renamed + if (specifier.propertyName && specifier.propertyName.text) { + return specifier.propertyName.text; + } + + return ''; + }; + + const changes = imports.map(p => { + const importSpecifiers = (p.importClause!.namedBindings! as ts.NamedImports) + .elements; + + const isAlreadyImported = importSpecifiers + .map(importText) + .includes(importToBe); + + const importChanges = importSpecifiers.map((specifier, index) => { + const text = importText(specifier); + + // import is not the one we're looking for, can be skipped + if (text !== importAsIs) { + return undefined; } - if (specifier.name.text === importAsIs) { - return { hit: true, specifier, text: specifier.name.text }; + // identifier has not been imported, simply replace the old text with the new text + if (!isAlreadyImported) { + return createReplaceChange( + sourceFile, + specifier!, + importAsIs, + importToBe + ); } - // if import is renamed - if ( - specifier.propertyName && - specifier.propertyName.text === importAsIs - ) { - return { hit: true, specifier, text: specifier.propertyName.text }; + const nextIdentifier = importSpecifiers[index + 1]; + // identifer is not the last, also clean up the comma + if (nextIdentifier) { + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + nextIdentifier.getStart(sourceFile) + ); } - return { hit: false }; - }) - .filter(({ hit }) => hit) - .map(({ specifier, text }) => - createReplaceChange(sourceFile, specifier!, text!, importToBe) - ); + // there are no imports following, just remove it + return createRemoveChange( + sourceFile, + specifier, + specifier.getStart(sourceFile), + specifier.getEnd() + ); + }); + + return importChanges.filter(Boolean) as (ReplaceChange | RemoveChange)[]; + }); - return changes; + return changes.reduce((imports, curr) => imports.concat(curr), []); } diff --git a/modules/store/schematics-core/utility/change.ts b/modules/store/schematics-core/utility/change.ts index 98c7fc6405..d4a22ae4a5 100644 --- a/modules/store/schematics-core/utility/change.ts +++ b/modules/store/schematics-core/utility/change.ts @@ -148,6 +148,15 @@ export function createReplaceChange( ); } +export function createRemoveChange( + sourceFile: ts.SourceFile, + node: ts.Node, + from = node.getStart(sourceFile), + to = node.getEnd() +): RemoveChange { + return new RemoveChange(sourceFile.fileName, from, to); +} + export function createChangeRecorder( tree: Tree, path: string,