From 9654443d4cc71ecb8805a3b6f7b2d4501ffed87f Mon Sep 17 00:00:00 2001 From: Richard Laffers Date: Wed, 16 Aug 2023 10:55:55 +0200 Subject: [PATCH] feat(no-inline-implementation): detect inline implementation inside spawn calls with xstate v4 Also prevent false positives on inline implementations within the second argument to createMachine. --- docs/rules/no-inline-implementation.md | 25 ++- lib/rules/no-inline-implementation.js | 26 ++- lib/rules/spawn-usage.js | 122 +----------- lib/utils/XStateDetector.js | 111 +++++++++++ tests/lib/rules/no-inline-implementation.js | 207 +++++++++++++++----- 5 files changed, 324 insertions(+), 167 deletions(-) create mode 100644 lib/utils/XStateDetector.js diff --git a/docs/rules/no-inline-implementation.md b/docs/rules/no-inline-implementation.md index 8e1b480..27279f2 100644 --- a/docs/rules/no-inline-implementation.md +++ b/docs/rules/no-inline-implementation.md @@ -34,7 +34,12 @@ createMachine({ actions: () => {} // inlined action } }, - activities: () => {} // inlined activity + activities: () => {}, // inlined activity + entry: assign({ + childActor: () => spawn( + () => {}, // inlined actor + ) + }), } } }) @@ -53,7 +58,12 @@ createMachine({ actions: huffAndPuff // defined elsewhere } }, - activities: beep // defined elsewhere + activities: beep, // defined elsewhere + entry: assign({ + childActor: () => spawn( + childMachine, // inlined actor + ) + }), } } }) @@ -77,11 +87,17 @@ createMachine( actions: ['huffAndPuff', 'log'] // arrays are ok too } }, - activities: 'beep' + activities: 'beep', + entry: assign({ + childActor: () => spawn( + 'childMachine', + ) + }), } } }, { + // "services" in XState v4, renamed to "actors" in XState v5 services: { someMachine: () => {} }, @@ -92,6 +108,7 @@ createMachine( huffAndPuff: () => {}, log: () => {} }, + // only in XState v4 activities: { beep: () => {} } @@ -106,7 +123,7 @@ createMachine({ entry: assign({ count: 1 }), on: { TRIGGER: { - actions: [assign({ count: 0 }), send('EVENT')] // arrays are ok too + actions: [assign({ count: 0 }), raise({ type: 'EVENT' })] // arrays are ok too } } } diff --git a/lib/rules/no-inline-implementation.js b/lib/rules/no-inline-implementation.js index 63532a7..d159860 100644 --- a/lib/rules/no-inline-implementation.js +++ b/lib/rules/no-inline-implementation.js @@ -6,13 +6,13 @@ const { isIdentifier, isStringLiteral, isCallExpression, - isObjectExpression, isKnownActionCreatorCall, isArrayExpression, } = require('../utils/predicates') const { getTypeProperty } = require('../utils/selectors') const { anyPass } = require('../utils/combinators') const getSettings = require('../utils/getSettings') +const XStateDetector = require('../utils/XStateDetector') function isArrayWithFunctionExpressionOrIdentifier(node) { return ( @@ -78,10 +78,10 @@ const activitiesProperty = 'CallExpression[callee.name=/^createMachine$|^Machine$/] Property[key.name="activities"]' const propertyOfChoosableActionObject = - 'CallExpression[callee.name=/^createMachine$|^Machine$/] CallExpression[callee.name="choose"] > ArrayExpression > ObjectExpression > Property' + 'CallExpression[callee.name=/^createMachine$|^Machine$/] > ObjectExpression:first-child CallExpression[callee.name="choose"] > ArrayExpression > ObjectExpression > Property' const propertyOfChoosableActionObjectAlt = - 'CallExpression[callee.name=/^createMachine$|^Machine$/] CallExpression[callee.type="MemberExpression"][callee.object.name="actions"][callee.property.name="choose"] > ArrayExpression > ObjectExpression > Property' + 'CallExpression[callee.name=/^createMachine$|^Machine$/] > ObjectExpression:first-child CallExpression[callee.type="MemberExpression"][callee.object.name="actions"][callee.property.name="choose"] > ArrayExpression > ObjectExpression > Property' const defaultOptions = { allowKnownActionCreators: false, @@ -110,7 +110,7 @@ module.exports = { type: 'suggestion', docs: { description: - 'Suggest refactoring actions, guards, services, activities into options', + 'Suggest refactoring actions, guards, actors, activities into options', category: 'Best Practices', url: getDocsUrl('no-inline-implementation'), recommended: 'warn', @@ -158,7 +158,8 @@ module.exports = { const { version } = getSettings(context) const options = context.options[0] || defaultOptions - // TODO also check what is passed to spawn + const xstateDetector = new XStateDetector() + function checkActorSrc(node) { if (isStringLiteral(node.value)) { return @@ -246,6 +247,7 @@ module.exports = { } return { + ...xstateDetector.visitors, [propertyOfEventTransition]: checkTransitionProperty, [propertyOfEventTransitionInArray]: checkTransitionProperty, [propertyOfAltEventTransitionInArray]: checkTransitionProperty, @@ -304,6 +306,20 @@ module.exports = { }) } }, + + 'CallExpression[callee.name=/^createMachine$|^Machine$/] > ObjectExpression:first-child CallExpression': + function (node) { + if (!xstateDetector.isSpawnCallExpression(node)) { + return + } + + if (node.arguments[0] && !isStringLiteral(node.arguments[0])) { + context.report({ + node: node.arguments[0], + messageId: 'moveActorToOptions', + }) + } + }, } }, } diff --git a/lib/rules/spawn-usage.js b/lib/rules/spawn-usage.js index 0e70df8..697b6fd 100644 --- a/lib/rules/spawn-usage.js +++ b/lib/rules/spawn-usage.js @@ -1,8 +1,8 @@ 'use strict' const getDocsUrl = require('../utils/getDocsUrl') - const { isFunctionExpression, isIIFE } = require('../utils/predicates') +const XStateDetector = require('../utils/XStateDetector') function isAssignCall(node) { return node.type === 'CallExpression' && node.callee.name === 'assign' @@ -54,124 +54,22 @@ module.exports = { }, create: function (context) { - // This will remain empty unless spawn is imported from the XState library - const spawnIdentifiers = [] - // This may get populated if the whole xstate module is imported - let xstateIdentifier = null - - function captureSpawnIdentifierFromXStateMemberExpression(node) { - if ( - xstateIdentifier != null && - node.init.object.name === xstateIdentifier - ) { - spawnIdentifiers.push(node.id.name) - } - } + const xstateDetector = new XStateDetector() return { - 'ImportDeclaration[source.value="xstate"]': function (node) { - const importSpecifier = node.specifiers.find( - (s) => s.type === 'ImportSpecifier' && s.imported.name === 'spawn' - ) - if (importSpecifier) { - spawnIdentifiers.push(importSpecifier.local.name) - return - } - const importDefaultSpecifier = node.specifiers.find( - (s) => s.type === 'ImportDefaultSpecifier' - ) - if (importDefaultSpecifier) { - xstateIdentifier = importDefaultSpecifier.local.name - return - } - const importNamespaceSpecifier = node.specifiers.find( - (s) => s.type === 'ImportNamespaceSpecifier' - ) - if (importNamespaceSpecifier) { - xstateIdentifier = importNamespaceSpecifier.local.name - } - }, - // commonjs imports - // const { spawn } = require('xstate') - // const { spawn: xspawn } = require('xstate') - // const xstate = require('xstate') - 'VariableDeclarator[init.callee.name="require"][init.arguments.0.value="xstate"]': - function (node) { - if (xstateIdentifier !== null || spawnIdentifiers.length > 0) { - return - } - if (node.id.type === 'ObjectPattern') { - const spawnProperty = node.id.properties.find( - (p) => p.key.name === 'spawn' - ) - if (spawnProperty) { - spawnIdentifiers.push(spawnProperty.value.name) - } - return - } - if (node.id.type === 'Identifier') { - xstateIdentifier = node.id.name - } - }, - // const spawn = require('xstate').spawn - // const spawn = require('xstate')['spawn'] - 'VariableDeclarator[init.object.callee.name="require"][init.object.arguments.0.value="xstate"]': - function (node) { - if ( - node.init.property.name === 'spawn' || - node.init.property.value === 'spawn' - ) { - spawnIdentifiers.push(node.id.name) - } - }, - // const { spawn } = xstate - // Ignores it if the xstateIdentifier has not been previously resolved - 'VariableDeclarator[id.type="ObjectPattern"] > ObjectPattern > Property[key.name="spawn"]': - function (node) { - const varDeclarator = node.parent.parent - if ( - xstateIdentifier != null && - xstateIdentifier === varDeclarator.init.name - ) { - spawnIdentifiers.push(node.value.name) - } - }, - // const spawn = xstate.spawn - // const spawn = xstate['spawn'] - 'VariableDeclarator[init.type="MemberExpression"][init.property.name="spawn"]': - captureSpawnIdentifierFromXStateMemberExpression, - 'VariableDeclarator[init.type="MemberExpression"][init.property.value="spawn"]': - captureSpawnIdentifierFromXStateMemberExpression, + ...xstateDetector.visitors, + // Ignores it if the spawnIdentifier has not been previously resolved to "spawn" (imported from xstate) CallExpression: function (node) { - if (spawnIdentifiers.length < 1 && xstateIdentifier == null) { + if (!xstateDetector.isSpawnCallExpression(node)) { return } - if ( - node.callee.type === 'Identifier' && - spawnIdentifiers.includes(node.callee.name) - ) { - if (!isInsideAssignerFunction(node)) { - context.report({ - node, - messageId: 'invalidCallContext', - }) - } - return - } - if ( - node.callee.type === 'MemberExpression' && - node.callee.object.name === xstateIdentifier && - (node.callee.property.name === 'spawn' || - node.callee.property.value === 'spawn') - ) { - if (!isInsideAssignerFunction(node)) { - context.report({ - node, - messageId: 'invalidCallContext', - }) - } + if (!isInsideAssignerFunction(node)) { + context.report({ + node, + messageId: 'invalidCallContext', + }) } }, } diff --git a/lib/utils/XStateDetector.js b/lib/utils/XStateDetector.js new file mode 100644 index 0000000..6441967 --- /dev/null +++ b/lib/utils/XStateDetector.js @@ -0,0 +1,111 @@ +/** + * XStateDetector can be used to capture information about xstate imports. + */ +module.exports = class XStateDetector { + constructor() { + this.xstateIdentifier = null + this.spawnIdentifiers = [] + + const captureSpawnIdentifierFromXStateMemberExpression = (node) => { + if ( + this.xstateIdentifier != null && + node.init.object.name === this.xstateIdentifier + ) { + this.spawnIdentifiers.push(node.id.name) + } + } + + this.visitors = { + 'ImportDeclaration[source.value="xstate"]': (node) => { + const importSpecifier = node.specifiers.find( + (s) => s.type === 'ImportSpecifier' && s.imported.name === 'spawn' + ) + if (importSpecifier) { + this.spawnIdentifiers.push(importSpecifier.local.name) + return + } + const importDefaultSpecifier = node.specifiers.find( + (s) => s.type === 'ImportDefaultSpecifier' + ) + if (importDefaultSpecifier) { + this.xstateIdentifier = importDefaultSpecifier.local.name + return + } + const importNamespaceSpecifier = node.specifiers.find( + (s) => s.type === 'ImportNamespaceSpecifier' + ) + if (importNamespaceSpecifier) { + this.xstateIdentifier = importNamespaceSpecifier.local.name + } + }, + // commonjs imports + // const { spawn } = require('xstate') + // const { spawn: xspawn } = require('xstate') + // const xstate = require('xstate') + 'VariableDeclarator[init.callee.name="require"][init.arguments.0.value="xstate"]': + (node) => { + if ( + this.xstateIdentifier !== null || + this.spawnIdentifiers.length > 0 + ) { + return + } + if (node.id.type === 'ObjectPattern') { + const spawnProperty = node.id.properties.find( + (p) => p.key.name === 'spawn' + ) + if (spawnProperty) { + this.spawnIdentifiers.push(spawnProperty.value.name) + } + return + } + if (node.id.type === 'Identifier') { + this.xstateIdentifier = node.id.name + } + }, + // const spawn = require('xstate').spawn + // const spawn = require('xstate')['spawn'] + 'VariableDeclarator[init.object.callee.name="require"][init.object.arguments.0.value="xstate"]': + (node) => { + if ( + node.init.property.name === 'spawn' || + node.init.property.value === 'spawn' + ) { + this.spawnIdentifiers.push(node.id.name) + } + }, + // const { spawn } = xstate + // Ignores it if the xstateIdentifier has not been previously resolved + 'VariableDeclarator[id.type="ObjectPattern"] > ObjectPattern > Property[key.name="spawn"]': + (node) => { + const varDeclarator = node.parent.parent + if ( + this.xstateIdentifier != null && + this.xstateIdentifier === varDeclarator.init.name + ) { + this.spawnIdentifiers.push(node.value.name) + } + }, + // const spawn = xstate.spawn + // const spawn = xstate['spawn'] + 'VariableDeclarator[init.type="MemberExpression"][init.property.name="spawn"]': + captureSpawnIdentifierFromXStateMemberExpression, + 'VariableDeclarator[init.type="MemberExpression"][init.property.value="spawn"]': + captureSpawnIdentifierFromXStateMemberExpression, + } + } + + isSpawnCallExpression(node) { + if (this.spawnIdentifiers.length < 1 && this.xstateIdentifier == null) { + return false + } + return ( + (node.callee.type === 'Identifier' && + this.spawnIdentifiers.includes(node.callee.name)) || + (node.callee.type === 'MemberExpression' && + node.callee.object.name === this.xstateIdentifier && + (node.callee.property.name === 'spawn' || + node.callee.property.value === 'spawn')) + ) + } +} diff --git a/tests/lib/rules/no-inline-implementation.js b/tests/lib/rules/no-inline-implementation.js index c77ece0..338cf3d 100644 --- a/tests/lib/rules/no-inline-implementation.js +++ b/tests/lib/rules/no-inline-implementation.js @@ -7,46 +7,79 @@ const tests = { withVersion( 4, ` - createMachine({ - states: { - active: { - invoke: { - src: 'myService', - }, - entry: 'myAction', - on: { - OFF: { - cond: 'myGuard', - target: 'inactive', - actions: ['myAction1', 'myAction2'], + createMachine( + { + states: { + active: { + invoke: { + src: 'myService', }, + entry: 'myAction', + on: { + OFF: { + cond: 'myGuard', + target: 'inactive', + actions: ['myAction1', 'myAction2'], + }, + }, + activities: 'myActivity', }, - activities: 'myActivity', }, - }, - }) + }, + { + services: { + myService: () => {}, + }, + actions: { + myAction: () => {}, + myAction1: () => {}, + myAction2: () => {}, + }, + guards: { + myGuard: () => {}, + }, + activities: { + myActivity: () => {}, + }, + } + ) ` ), withVersion( 5, ` - createMachine({ - states: { - active: { - invoke: { - src: 'myService', - }, - entry: 'myAction', - on: { - OFF: { - guard: 'myGuard', - target: 'inactive', - actions: ['myAction1', 'myAction2'], + createMachine( + { + states: { + active: { + invoke: { + src: 'myActor', + }, + entry: 'myAction', + on: { + OFF: { + guard: 'myGuard', + target: 'inactive', + actions: ['myAction1', 'myAction2'], + }, }, }, }, }, - }) + { + actors: { + myActor: () => {}, + }, + actions: { + myAction: () => {}, + myAction1: () => {}, + myAction2: () => {}, + }, + guards: { + myGuard: () => {}, + }, + } + ) ` ), // inlined action creators are ok with allowKnownActionCreators=true @@ -54,36 +87,82 @@ const tests = { 4, ` /* eslint no-inline-implementation: [ "warn", { "allowKnownActionCreators": true } ] */ - createMachine({ - states: { - active: { - entry: assign(), - on: { - OFF: { - actions: [send('EVENT'), assign()], + createMachine( + { + states: { + active: { + entry: assign({ + childActor: () => spawn('childActor'), + }), + on: { + OFF: { + actions: [send('EVENT'), assign()], + }, }, + exit: choose([{ + cond: 'myGuard', + actions: 'myAction', + }]), }, }, }, - }) + { + services: { + childActor: () => {}, + }, + guards: { + myGuard: () => {}, + }, + actions: { + myAction: () => {}, + choosableAction: choose([{ + cond: () => {}, + actions: () => {}, + }]), + }, + } + ) ` ), withVersion( 5, ` /* eslint no-inline-implementation: [ "warn", { "allowKnownActionCreators": true } ] */ - createMachine({ - states: { - active: { - entry: assign(), - on: { - OFF: { - actions: [sendParent('EVENT'), assign()], + createMachine( + { + states: { + active: { + entry: assign({ + childActor: () => spawn('childActor'), + }), + on: { + OFF: { + actions: [sendParent('EVENT'), assign()], + }, }, + exit: choose([{ + guard: 'myGuard', + actions: 'myAction', + }]), }, }, }, - }) + { + actors: { + childActor: () => {}, + }, + guards: { + myGuard: () => {}, + }, + actions: { + myAction: () => {}, + choosableAction: choose([{ + guard: () => {}, + actions: () => {}, + }]), + }, + } + ) ` ), // onDone, onError, array of transitions @@ -434,16 +513,30 @@ const tests = { withVersion(4, { code: ` /* eslint no-inline-implementation: [ "warn", { "allowKnownActionCreators": true } ] */ + const { createMachine, spawn } = require('xstate') createMachine({ states: { active: { entry: ['someAction', assign(), () => {}], on: { OFF: { - actions: ['someAction', someAction, () => {}, send()], + actions: [ + 'someAction', + someAction, + () => {}, + send(), + choose([{ + cond: () => {}, + actions: () => {}, + }]), + ], }, }, activities: [myActivity, 'myActivity'], + exit: assign({ + childActor1: () => spawn(() => {}), + childActor2: () => spawn(childActor), + }), }, }, }) @@ -452,21 +545,39 @@ const tests = { { messageId: 'moveActionToOptions' }, { messageId: 'moveActionToOptions' }, { messageId: 'moveActionToOptions' }, + { messageId: 'moveGuardToOptions' }, + { messageId: 'moveActionToOptions' }, { messageId: 'moveActivityToOptions' }, + { messageId: 'moveActorToOptions' }, + { messageId: 'moveActorToOptions' }, ], }), withVersion(5, { code: ` /* eslint no-inline-implementation: [ "warn", { "allowKnownActionCreators": true } ] */ + const { createMachine, spawn } = require('xstate') createMachine({ states: { active: { entry: ['someAction', assign(), () => {}], on: { OFF: { - actions: ['someAction', someAction, () => {}, sendTo()], + actions: [ + 'someAction', + someAction, + () => {}, + sendParent(), + choose([{ + guard: () => {}, + actions: () => {}, + }]), + ], }, }, + exit: assign({ + childActor1: () => spawn(() => {}), + childActor2: () => spawn(childActor), + }), }, }, }) @@ -475,6 +586,10 @@ const tests = { { messageId: 'moveActionToOptions' }, { messageId: 'moveActionToOptions' }, { messageId: 'moveActionToOptions' }, + { messageId: 'moveGuardToOptions' }, + { messageId: 'moveActionToOptions' }, + { messageId: 'moveActorToOptions' }, + { messageId: 'moveActorToOptions' }, ], }), // inline implementations inside array of transitions