Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check action objects for implementation use #395

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/lazy-ads-warn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@xstate/vscode-server': patch
---

Action objects (`{ type: 'action1' }`) are now checked as part of the unused action implementation algorithm.

This resolves an issue where action objects could reference a valid implementation but the extension reported a warning that the implementation was unused.
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { extractMachinesFromFile } from '@xstate/machine-extractor';
import { DiagnosticSeverity } from 'vscode-languageserver';
import { getUnusedActionImplementations } from '../diagnostics/getUnusedActionImplementations';

test('returns a Diagnostic for each unused action in the machine', () => {
const result = extractMachinesFromFile(`
createMachine(
{},
{
actions: {
action1: () => {},
action2: () => {},
},
},
);
`);
const diagnostics = getUnusedActionImplementations(
result!.machines[0]!,
undefined!,
undefined!,
);

expect(diagnostics).toHaveLength(2);

for (const action of ['action1', 'action2']) {
expect(diagnostics).toContainEqual({
message: `${action} is never used in the machine definition`,
range: {
start: {
character: expect.any(Number),
line: expect.any(Number),
},
end: {
character: expect.any(Number),
line: expect.any(Number),
},
},
severity: DiagnosticSeverity.Warning,
});
}
});

test('does not return a Diagnostic for actions used in the machine', () => {
const result = extractMachinesFromFile(`
createMachine(
{
entry: ['action1', { type: 'action2' }],
},
{
actions: {
action1: () => {},
action2: () => {},
},
},
);
`);
const diagnostics = getUnusedActionImplementations(
result!.machines[0]!,
undefined!,
undefined!,
);

expect(diagnostics).toHaveLength(0);
});
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ export const getUnusedActionImplementations: DiagnosticGetter = (
machineResult,
) => {
const allActions = getSetOfNames(
machineResult.getAllActions(['named']) || [],
machineResult.getAllActions(['named', 'object']) || [],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'object' declaration type doesn't exist:

declarationTypes: DeclarationType[] = [
'identifier',
'inline',
'unknown',
'named',
],

was this maybe intended as a draft PR? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does exist, that default array is just confusing

export type DeclarationType =
| 'named'
| 'inline'
| 'identifier'
| 'object'
| 'unknown';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used over here

export const ActionAsObjectExpression = createParser({
babelMatcher: t.isObjectExpression,
parseNode: (node, context): ActionNode => {
const id = context.getNodeHash(node);
for (const prop of node.properties) {
if (t.isObjectProperty(prop)) {
if (
getObjectPropertyKey(prop) === 'type' &&
t.isStringLiteral(prop.value) &&
!SUPPORTED_BUILTIN_ACTIONS.includes(prop.value.value)
) {
return {
action: id,
node,
name: prop.value.value,
kind: 'named',
declarationType: 'object',
inlineDeclarationId: id,
};
}
}
}

);

const unusedActions =