Skip to content

Commit 13c4832

Browse files
paoloricciutipaoloricciuti
authored
Merge pull request #7 from sveltejs/autofixer-folders-and-tests
2 parents 5becaf3 + 7c7a1f9 commit 13c4832

File tree

11 files changed

+274
-91
lines changed

11 files changed

+274
-91
lines changed

‎src/lib/mcp/autofixers.ts‎

Lines changed: 0 additions & 47 deletions
This file was deleted.

‎src/lib/mcp/autofixers/add-autofixers-issues.test.ts‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
import { describe, expect, it } from 'vitest';
2+
import { add_autofixers_issues } from './add-autofixers-issues';
3+
4+
describe('add_autofixers_issues', () => {
5+
describe('assign_in_effect', () => {
6+
it('should add suggestions when assigning to a stateful variable inside an effect', () => {
7+
const content = { issues: [], suggestions: [] };
8+
const code = `
9+
<script>
10+
const count = $state(0);
11+
$effect(() => {
12+
count = 43;
13+
});
14+
</script>`;
15+
add_autofixers_issues(content, code, 5);
16+
17+
expect(content.suggestions.length).toBeGreaterThanOrEqual(1);
18+
expect(content.suggestions).toContain(
19+
'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
20+
);
21+
});
22+
23+
it('should add a suggestion for each variable assigned within an effect', () => {
24+
const content = { issues: [], suggestions: [] };
25+
const code = `
26+
<script>
27+
const count = $state(0);
28+
const count2 = $state(0);
29+
$effect(() => {
30+
count = 43;
31+
count2 = 44;
32+
});
33+
</script>`;
34+
add_autofixers_issues(content, code, 5);
35+
36+
expect(content.suggestions.length).toBeGreaterThanOrEqual(2);
37+
expect(content.suggestions).toContain(
38+
'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
39+
);
40+
expect(content.suggestions).toContain(
41+
'The stateful variable "count2" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
42+
);
43+
});
44+
it('should not add a suggestion for variables that are not assigned within an effect', () => {
45+
const content = { issues: [], suggestions: [] };
46+
const code = `
47+
<script>
48+
const count = $state(0);
49+
</script>
50+
51+
<button onclick={() => count = 43}>Increment</button>
52+
`;
53+
add_autofixers_issues(content, code, 5);
54+
55+
expect(content.suggestions).not.toContain(
56+
'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
57+
);
58+
});
59+
60+
it("should not add a suggestions for variables that are assigned within an effect but aren't stateful", () => {
61+
const content = { issues: [], suggestions: [] };
62+
const code = `
63+
<script>
64+
const count = 0;
65+
66+
$effect(() => {
67+
count = 43;
68+
});
69+
</script>`;
70+
add_autofixers_issues(content, code, 5);
71+
72+
expect(content.suggestions).not.toContain(
73+
'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
74+
);
75+
});
76+
77+
it('should add a suggestion for variables that are assigned within an effect with an update', () => {
78+
const content = { issues: [], suggestions: [] };
79+
const code = `
80+
<script>
81+
let count = $state(0);
82+
83+
$effect(() => {
84+
count++;
85+
});
86+
</script>
87+
`;
88+
add_autofixers_issues(content, code, 5);
89+
90+
expect(content.suggestions).toContain(
91+
'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
92+
);
93+
});
94+
95+
it('should add a suggestion for variables that are mutated within an effect', () => {
96+
const content = { issues: [], suggestions: [] };
97+
const code = `
98+
<script>
99+
let count = $state({ value: 0 });
100+
101+
$effect(() => {
102+
count.value = 42;
103+
});
104+
</script>
105+
`;
106+
add_autofixers_issues(content, code, 5);
107+
108+
expect(content.suggestions).toContain(
109+
'The stateful variable "count" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.',
110+
);
111+
});
112+
});
113+
});

‎src/lib/mcp/autofixers/add-autofixers-issues.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { parse } from '../../parse/parse.js';
2+
import { walk } from '../../index.js';
3+
import type { Node } from 'estree';
4+
import * as autofixers from './visitors/index.js';
5+
6+
export function add_autofixers_issues(
7+
content: { issues: string[]; suggestions: string[] },
8+
code: string,
9+
desired_svelte_version: number,
10+
filename = 'Component.svelte',
11+
) {
12+
const parsed = parse(code, filename);
13+
14+
// Run each autofixer separately to avoid interrupting logic flow
15+
for (const autofixer of Object.values(autofixers)) {
16+
walk(
17+
parsed.ast as unknown as Node,
18+
{ output: content, parsed, desired_svelte_version },
19+
autofixer,
20+
);
21+
}
22+
}

‎src/lib/mcp/autofixers/add-compile-issues.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { compile } from 'svelte/compiler';
2+
3+
export function add_compile_issues(
4+
content: { issues: string[]; suggestions: string[] },
5+
code: string,
6+
desired_svelte_version: number,
7+
filename = 'Component.svelte',
8+
) {
9+
const compilation_result = compile(code, {
10+
filename: filename || 'Component.svelte',
11+
generate: false,
12+
runes: desired_svelte_version >= 5,
13+
});
14+
15+
for (const warning of compilation_result.warnings) {
16+
content.issues.push(
17+
`${warning.message} at line ${warning.start?.line}, column ${warning.start?.column}`,
18+
);
19+
}
20+
}

src/lib/mcp/eslint.ts renamed to src/lib/mcp/autofixers/add-eslint-issues.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ function base_config(svelte_config: Config): ESLint.Options['baseConfig'] {
4848
];
4949
}
5050

51-
export function get_linter(version: number) {
51+
function get_linter(version: number) {
5252
if (version < 5) {
5353
return (svelte_4_linter ??= new ESLint({
5454
overrideConfigFile: true,
@@ -68,3 +68,23 @@ export function get_linter(version: number) {
6868
}),
6969
}));
7070
}
71+
72+
export async function add_eslint_issues(
73+
content: { issues: string[]; suggestions: string[] },
74+
code: string,
75+
desired_svelte_version: number,
76+
filename = 'Component.svelte',
77+
) {
78+
const eslint = get_linter(desired_svelte_version);
79+
const results = await eslint.lintText(code, { filePath: filename || './Component.svelte' });
80+
81+
for (const message of results[0].messages) {
82+
if (message.severity === 2) {
83+
content.issues.push(`${message.message} at line ${message.line}, column ${message.column}`);
84+
} else if (message.severity === 1) {
85+
content.suggestions.push(
86+
`${message.message} at line ${message.line}, column ${message.column}`,
87+
);
88+
}
89+
}
90+
}

‎src/lib/mcp/autofixers/ast/utils.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import type { Identifier, MemberExpression } from 'estree';
2+
3+
/**
4+
* Gets the left-most identifier of a member expression or identifier.
5+
*/
6+
export function left_most_id(expression: MemberExpression | Identifier) {
7+
while (expression.type === 'MemberExpression') {
8+
expression = expression.object as MemberExpression | Identifier;
9+
}
10+
11+
if (expression.type !== 'Identifier') {
12+
return null;
13+
}
14+
15+
return expression;
16+
}

‎src/lib/mcp/autofixers/visitors/assign-in-effect.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import type { AssignmentExpression, Identifier, Node, UpdateExpression } from 'estree';
2+
import type { Autofixer, AutofixerState } from '.';
3+
import { left_most_id } from '../ast/utils';
4+
import type { SvelteNode } from 'svelte-eslint-parser/lib/ast';
5+
import type { Context } from 'zimmerframe';
6+
7+
function run_if_in_effect(path: (Node | SvelteNode)[], state: AutofixerState, to_run: () => void) {
8+
const in_effect = path.findLast(
9+
(node) =>
10+
node.type === 'CallExpression' &&
11+
node.callee.type === 'Identifier' &&
12+
node.callee.name === '$effect',
13+
);
14+
15+
if (
16+
in_effect &&
17+
in_effect.type === 'CallExpression' &&
18+
(in_effect.callee.type === 'Identifier' || in_effect.callee.type === 'MemberExpression')
19+
) {
20+
if (state.parsed.is_rune(in_effect, ['$effect', '$effect.pre'])) {
21+
to_run();
22+
}
23+
}
24+
}
25+
26+
function visitor(
27+
node: UpdateExpression | AssignmentExpression,
28+
{ state, path }: Context<Node | SvelteNode, AutofixerState>,
29+
) {
30+
run_if_in_effect(path, state, () => {
31+
function check_if_stateful_id(id: Identifier) {
32+
const reference = state.parsed.find_reference_by_id(id);
33+
const definition = reference?.resolved?.defs[0];
34+
if (definition && definition.type === 'Variable') {
35+
const init = definition.node.init;
36+
if (
37+
init?.type === 'CallExpression' &&
38+
state.parsed.is_rune(init, ['$state', '$state.raw'])
39+
) {
40+
state.output.suggestions.push(
41+
`The stateful variable "${id.name}" is assigned inside an $effect which is generally consider a malpractice. Consider using $derived if possible.`,
42+
);
43+
}
44+
}
45+
}
46+
const variable = node.type === 'UpdateExpression' ? node.argument : node.left;
47+
48+
if (variable.type === 'Identifier') {
49+
check_if_stateful_id(variable);
50+
} else if (variable.type === 'MemberExpression') {
51+
const object = left_most_id(variable);
52+
if (object) {
53+
check_if_stateful_id(object);
54+
}
55+
}
56+
});
57+
}
58+
59+
export const assign_in_effect: Autofixer = {
60+
UpdateExpression: visitor,
61+
AssignmentExpression: visitor,
62+
};

‎src/lib/mcp/autofixers/visitors/index.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import type { Node } from 'estree';
2+
import type { AST } from 'svelte-eslint-parser';
3+
import type { Visitors } from 'zimmerframe';
4+
import type { ParseResult } from '../../../parse/parse.js';
5+
6+
export type AutofixerState = {
7+
output: { issues: string[]; suggestions: string[] };
8+
parsed: ParseResult;
9+
desired_svelte_version: number;
10+
};
11+
12+
export type Autofixer = Visitors<Node | AST.SvelteNode, AutofixerState>;
13+
14+
export * from './assign-in-effect.js';

0 commit comments

Comments
 (0)