From 83951657acfed4bb788120b15c588620f08774af Mon Sep 17 00:00:00 2001 From: Yuichiro Yamashita Date: Sat, 3 Sep 2022 06:23:36 +0000 Subject: [PATCH 1/4] feat: implement derived-has-same-inputs-outputs rule --- README.md | 1 + docs/rules.md | 1 + docs/rules/derived-has-same-inputs-outputs.md | 48 ++++++++ src/rules/derived-has-same-inputs-outputs.ts | 110 ++++++++++++++++++ src/rules/reference-helpers/svelte-store.ts | 9 +- src/utils/rules.ts | 2 + .../invalid/test01-errors.yaml | 24 ++++ .../invalid/test01-input.js | 14 +++ .../valid/test01-input.js | 14 +++ .../rules/derived-has-same-inputs-outputs.ts | 16 +++ 10 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 docs/rules/derived-has-same-inputs-outputs.md create mode 100644 src/rules/derived-has-same-inputs-outputs.ts create mode 100644 tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml create mode 100644 tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js create mode 100644 tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js create mode 100644 tests/src/rules/derived-has-same-inputs-outputs.ts diff --git a/README.md b/README.md index 9f78407f7..d14be6352 100644 --- a/README.md +++ b/README.md @@ -289,6 +289,7 @@ These rules relate to better ways of doing things to help you avoid problems: | Rule ID | Description | | |:--------|:------------|:---| | [svelte/button-has-type](https://ota-meshi.github.io/eslint-plugin-svelte/rules/button-has-type/) | disallow usage of button without an explicit type attribute | | +| [svelte/derived-has-same-inputs-outputs](https://ota-meshi.github.io/eslint-plugin-svelte/rules/derived-has-same-inputs-outputs/) | derived store should use same variable names between values and callback | | | [svelte/no-at-debug-tags](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-at-debug-tags/) | disallow the use of `{@debug}` | :star: | | [svelte/no-reactive-functions](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-reactive-functions/) | it's not necessary to define functions in reactive statements | :bulb: | | [svelte/no-reactive-literals](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-reactive-literals/) | don't assign literal values in reactive statements | :bulb: | diff --git a/docs/rules.md b/docs/rules.md index 1bf79427c..a335ce2b9 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -42,6 +42,7 @@ These rules relate to better ways of doing things to help you avoid problems: | Rule ID | Description | | |:--------|:------------|:---| | [svelte/button-has-type](./rules/button-has-type.md) | disallow usage of button without an explicit type attribute | | +| [svelte/derived-has-same-inputs-outputs](./rules/derived-has-same-inputs-outputs.md) | derived store should use same variable names between values and callback | | | [svelte/no-at-debug-tags](./rules/no-at-debug-tags.md) | disallow the use of `{@debug}` | :star: | | [svelte/no-reactive-functions](./rules/no-reactive-functions.md) | it's not necessary to define functions in reactive statements | :bulb: | | [svelte/no-reactive-literals](./rules/no-reactive-literals.md) | don't assign literal values in reactive statements | :bulb: | diff --git a/docs/rules/derived-has-same-inputs-outputs.md b/docs/rules/derived-has-same-inputs-outputs.md new file mode 100644 index 000000000..e63e488c0 --- /dev/null +++ b/docs/rules/derived-has-same-inputs-outputs.md @@ -0,0 +1,48 @@ +--- +pageClass: "rule-details" +sidebarDepth: 0 +title: "svelte/derived-has-same-inputs-outputs" +description: "derived store should use same variable names between values and callback" +--- + +# svelte/derived-has-same-inputs-outputs + +> derived store should use same variable names between values and callback + +## :book: Rule Details + +This rule reports where variable names and callback function's argument names are different. +This is mainly a recommended rule to avoid implementation confusion. + + + + + +```js +/* eslint svelte/derived-has-same-inputs-outputs: "error" */ + +import { derived } from "svelte/store" + +/* ✓ GOOD */ +derived(a, ($a) => {}); +derived(a, ($a, set) => {}) +derived([ a, b ], ([ $a, $b ]) => {}) + +/* ✗ BAD */ +derived(a, (b) => {}); +derived(a, (b, set) => {}); +derived([ a, b ], ([ one, two ]) => {}) +``` + + + +## :wrench: Options + +Nothing. + + +## :books: Further Reading + +- [Svelte - Docs > RUN TIME > svelte/store > derived](https://svelte.dev/docs#run-time-svelte-store-derived) + + diff --git a/src/rules/derived-has-same-inputs-outputs.ts b/src/rules/derived-has-same-inputs-outputs.ts new file mode 100644 index 000000000..912defd81 --- /dev/null +++ b/src/rules/derived-has-same-inputs-outputs.ts @@ -0,0 +1,110 @@ +import type * as ESTree from "estree" +import { createRule } from "../utils" +import type { RuleContext } from "../types" +import { extractStoreReferences } from "./reference-helpers/svelte-store" + +export default createRule("derived-has-same-inputs-outputs", { + meta: { + docs: { + description: "", + category: "Best Practices", + recommended: false, + }, + schema: [], + messages: { + unexpected: "The argument name should be '{{name}}'.", + }, + type: "suggestion", + }, + create(context) { + /** check node type */ + function isIdentifierOrArrayExpression( + node: ESTree.SpreadElement | ESTree.Expression, + ): node is ESTree.Identifier | ESTree.ArrayExpression { + return ["Identifier", "ArrayExpression"].includes(node.type) + } + + type ArrowFunctionExpressionOrFunctionExpression = + | ESTree.ArrowFunctionExpression + | ESTree.FunctionExpression + + /** check node type */ + function isFunctionExpression( + node: ESTree.SpreadElement | ESTree.Expression, + ): node is ArrowFunctionExpressionOrFunctionExpression { + return ["ArrowFunctionExpression", "FunctionExpression"].includes( + node.type, + ) + } + + /** + * Check for identifier type. + * e.g. derived(a, ($a) => {}); + */ + function checkIdentifier( + context: RuleContext, + args: ESTree.Identifier, + fn: ArrowFunctionExpressionOrFunctionExpression, + ) { + const fnParam = fn.params[0] + if (fnParam.type !== "Identifier") return + const expectedName = `$${args.name}` + if (expectedName !== fnParam.name) { + context.report({ + node: fn, + loc: { + start: fnParam.loc?.start ?? { line: 1, column: 0 }, + end: fnParam.loc?.end ?? { line: 1, column: 0 }, + }, + messageId: "unexpected", + data: { name: expectedName }, + }) + } + } + + /** + * Check for array type. + * e.g. derived([ a, b ], ([ $a, $b ]) => {}) + */ + function checkArrayExpression( + context: RuleContext, + args: ESTree.ArrayExpression, + fn: ArrowFunctionExpressionOrFunctionExpression, + ) { + const fnParam = fn.params[0] + if (fnParam.type !== "ArrayPattern") return + const argNames = args.elements.map((element) => { + return element && element.type === "Identifier" ? element.name : null + }) + fnParam.elements.forEach((element, index) => { + if (element && element.type === "Identifier") { + const expectedName = `$${argNames[index]}` + if (expectedName !== element.name) { + context.report({ + node: fn, + loc: { + start: element.loc?.start ?? { line: 1, column: 0 }, + end: element.loc?.end ?? { line: 1, column: 0 }, + }, + messageId: "unexpected", + data: { name: expectedName }, + }) + } + } + }) + } + + return { + Program() { + for (const { node } of extractStoreReferences(context, ["derived"])) { + const [args, fn] = node.arguments + if (!args || !isIdentifierOrArrayExpression(args)) continue + if (!fn || !isFunctionExpression(fn)) continue + if (!fn.params || fn.params.length === 0) continue + if (args.type === "Identifier") checkIdentifier(context, args, fn) + else checkArrayExpression(context, args, fn) + } + }, + } + }, +}) diff --git a/src/rules/reference-helpers/svelte-store.ts b/src/rules/reference-helpers/svelte-store.ts index 3e9edc975..ba58abff3 100644 --- a/src/rules/reference-helpers/svelte-store.ts +++ b/src/rules/reference-helpers/svelte-store.ts @@ -2,22 +2,25 @@ import type * as ESTree from "estree" import { ReferenceTracker } from "eslint-utils" import type { RuleContext } from "../../types" +type StoreName = "writable" | "readable" | "derived" + /** Extract 'svelte/store' references */ export function* extractStoreReferences( context: RuleContext, + storeNames: StoreName[] = ["writable", "readable", "derived"], ): Generator<{ node: ESTree.CallExpression; name: string }, void> { const referenceTracker = new ReferenceTracker(context.getScope()) for (const { node, path } of referenceTracker.iterateEsmReferences({ "svelte/store": { [ReferenceTracker.ESM]: true, writable: { - [ReferenceTracker.CALL]: true, + [ReferenceTracker.CALL]: storeNames.includes("writable"), }, readable: { - [ReferenceTracker.CALL]: true, + [ReferenceTracker.CALL]: storeNames.includes("readable"), }, derived: { - [ReferenceTracker.CALL]: true, + [ReferenceTracker.CALL]: storeNames.includes("derived"), }, }, })) { diff --git a/src/utils/rules.ts b/src/utils/rules.ts index 9efb8fcae..f22699f28 100644 --- a/src/utils/rules.ts +++ b/src/utils/rules.ts @@ -1,6 +1,7 @@ import type { RuleModule } from "../types" import buttonHasType from "../rules/button-has-type" import commentDirective from "../rules/comment-directive" +import derivedHasSameInputsOutputs from "../rules/derived-has-same-inputs-outputs" import firstAttributeLinebreak from "../rules/first-attribute-linebreak" import htmlClosingBracketSpacing from "../rules/html-closing-bracket-spacing" import htmlQuotes from "../rules/html-quotes" @@ -41,6 +42,7 @@ import validCompile from "../rules/valid-compile" export const rules = [ buttonHasType, commentDirective, + derivedHasSameInputsOutputs, firstAttributeLinebreak, htmlClosingBracketSpacing, htmlQuotes, diff --git a/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml new file mode 100644 index 000000000..373647d9a --- /dev/null +++ b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml @@ -0,0 +1,24 @@ +- message: The argument name should be '$a'. + line: 3 + column: 13 + suggestions: null +- message: The argument name should be '$c'. + line: 6 + column: 13 + suggestions: null +- message: The argument name should be '$e'. + line: 9 + column: 19 + suggestions: null +- message: The argument name should be '$f'. + line: 9 + column: 22 + suggestions: null +- message: The argument name should be '$i'. + line: 12 + column: 19 + suggestions: null +- message: The argument name should be '$j'. + line: 12 + column: 22 + suggestions: null diff --git a/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js new file mode 100644 index 000000000..104e1564f --- /dev/null +++ b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js @@ -0,0 +1,14 @@ +import { derived } from "svelte/store" + +derived(a, (b) => { + /** do nothing */ +}) +derived(c, (d, set) => { + /** do nothing */ +}) +derived([e, f], ([g, h]) => { + /** do nothing */ +}) +derived([i, j], ([k, l], set) => { + /** do nothing */ +}) diff --git a/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js b/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js new file mode 100644 index 000000000..b4c07115c --- /dev/null +++ b/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js @@ -0,0 +1,14 @@ +import { derived } from "svelte/store" + +derived(a, ($a) => { + /** do nothing */ +}) +derived(c, ($c, set) => { + /** do nothing */ +}) +derived([e, f], ([$e, $f]) => { + /** do nothing */ +}) +derived([i, j], ([$i, $j], set) => { + /** do nothing */ +}) diff --git a/tests/src/rules/derived-has-same-inputs-outputs.ts b/tests/src/rules/derived-has-same-inputs-outputs.ts new file mode 100644 index 000000000..8c05e2b60 --- /dev/null +++ b/tests/src/rules/derived-has-same-inputs-outputs.ts @@ -0,0 +1,16 @@ +import { RuleTester } from "eslint" +import rule from "../../../src/rules/derived-has-same-inputs-outputs" +import { loadTestCases } from "../../utils/utils" + +const tester = new RuleTester({ + parserOptions: { + ecmaVersion: 2020, + sourceType: "module", + }, +}) + +tester.run( + "derived-has-same-inputs-outputs", + rule as any, + loadTestCases("derived-has-same-inputs-outputs"), +) From 69d57b1a4290fe98836c24145b5f8b2ce5f3d951 Mon Sep 17 00:00:00 2001 From: Yuichiro Yamashita Date: Mon, 5 Sep 2022 09:16:02 +0900 Subject: [PATCH 2/4] chore: add changeset --- .changeset/gentle-bugs-stare.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/gentle-bugs-stare.md diff --git a/.changeset/gentle-bugs-stare.md b/.changeset/gentle-bugs-stare.md new file mode 100644 index 000000000..e207c8196 --- /dev/null +++ b/.changeset/gentle-bugs-stare.md @@ -0,0 +1,5 @@ +--- +"eslint-plugin-svelte": minor +--- + +feat: add `svelte/derived-has-same-inputs-outputs` rule From ba6dcccb7f8c182895f8084b2f951c4087aeaaa1 Mon Sep 17 00:00:00 2001 From: Yuichiro Yamashita Date: Mon, 5 Sep 2022 15:46:54 +0900 Subject: [PATCH 3/4] chore: update according to review comments --- src/rules/derived-has-same-inputs-outputs.ts | 21 +++++++++----------- src/rules/no-store-async.ts | 2 +- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/src/rules/derived-has-same-inputs-outputs.ts b/src/rules/derived-has-same-inputs-outputs.ts index 912defd81..1d9a7a328 100644 --- a/src/rules/derived-has-same-inputs-outputs.ts +++ b/src/rules/derived-has-same-inputs-outputs.ts @@ -6,9 +6,11 @@ import { extractStoreReferences } from "./reference-helpers/svelte-store" export default createRule("derived-has-same-inputs-outputs", { meta: { docs: { - description: "", - category: "Best Practices", + description: + "derived store should use same variable names between values and callback", + category: "Stylistic Issues", recommended: false, + conflictWithPrettier: false, }, schema: [], messages: { @@ -52,10 +54,7 @@ export default createRule("derived-has-same-inputs-outputs", { if (expectedName !== fnParam.name) { context.report({ node: fn, - loc: { - start: fnParam.loc?.start ?? { line: 1, column: 0 }, - end: fnParam.loc?.end ?? { line: 1, column: 0 }, - }, + loc: fnParam.loc!, messageId: "unexpected", data: { name: expectedName }, }) @@ -77,15 +76,13 @@ export default createRule("derived-has-same-inputs-outputs", { return element && element.type === "Identifier" ? element.name : null }) fnParam.elements.forEach((element, index) => { - if (element && element.type === "Identifier") { - const expectedName = `$${argNames[index]}` + const argName = argNames[index] + if (element && element.type === "Identifier" && argName) { + const expectedName = `$${argName}` if (expectedName !== element.name) { context.report({ node: fn, - loc: { - start: element.loc?.start ?? { line: 1, column: 0 }, - end: element.loc?.end ?? { line: 1, column: 0 }, - }, + loc: element.loc!, messageId: "unexpected", data: { name: expectedName }, }) diff --git a/src/rules/no-store-async.ts b/src/rules/no-store-async.ts index 6934b697e..b8dc13ea5 100644 --- a/src/rules/no-store-async.ts +++ b/src/rules/no-store-async.ts @@ -32,7 +32,7 @@ export default createRule("no-store-async", { continue } - const start = fn.loc?.start ?? { line: 1, column: 0 } + const start = fn.loc!.start context.report({ node: fn, loc: { From 02da48269fb702067d2705441a7d4c904289320f Mon Sep 17 00:00:00 2001 From: Yuichiro Yamashita Date: Mon, 5 Sep 2022 16:00:06 +0900 Subject: [PATCH 4/4] chore: add more tests --- .../invalid/test01-errors.yaml | 8 ++++++++ .../invalid/test01-input.js | 6 ++++++ .../valid/test01-input.js | 12 ++++++++++++ 3 files changed, 26 insertions(+) diff --git a/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml index 373647d9a..9ce09c034 100644 --- a/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml +++ b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-errors.yaml @@ -22,3 +22,11 @@ line: 12 column: 22 suggestions: null +- message: The argument name should be '$l'. + line: 15 + column: 26 + suggestions: null +- message: The argument name should be '$o'. + line: 18 + column: 22 + suggestions: null diff --git a/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js index 104e1564f..e272e9579 100644 --- a/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js +++ b/tests/fixtures/rules/derived-has-same-inputs-outputs/invalid/test01-input.js @@ -12,3 +12,9 @@ derived([e, f], ([g, h]) => { derived([i, j], ([k, l], set) => { /** do nothing */ }) +derived([null, l], ([$m, $n]) => { + /** do nothing */ +}) +derived([o, null], ([$p, $q]) => { + /** do nothing */ +}) diff --git a/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js b/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js index b4c07115c..40e0f2db1 100644 --- a/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js +++ b/tests/fixtures/rules/derived-has-same-inputs-outputs/valid/test01-input.js @@ -12,3 +12,15 @@ derived([e, f], ([$e, $f]) => { derived([i, j], ([$i, $j], set) => { /** do nothing */ }) +derived(null, ($null, set) => { + /** do nothing */ +}) +derived(null, ($k, set) => { + /** do nothing */ +}) +derived([null, l], ([$m, $l]) => { + /** do nothing */ +}) +derived([n, null], ([$n, $o]) => { + /** do nothing */ +})