Skip to content

Add prefer-destructured-store-props rule #208

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

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ module.exports = {
},
{
files: ["*.svelte"],
extends: ["plugin:svelte/base"],
parser: "svelte-eslint-parser",
parserOptions: {
parser: {
Expand Down
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ These rules relate to better ways of doing things to help you avoid problems:
| [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: |
| [svelte/no-unused-svelte-ignore](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-unused-svelte-ignore/) | disallow unused svelte-ignore comments | :star: |
| [svelte/no-useless-mustaches](https://ota-meshi.github.io/eslint-plugin-svelte/rules/no-useless-mustaches/) | disallow unnecessary mustache interpolations | :wrench: |
| [svelte/prefer-destructured-store-props](https://ota-meshi.github.io/eslint-plugin-svelte/rules/prefer-destructured-store-props/) | (no description) | |
| [svelte/require-optimized-style-attribute](https://ota-meshi.github.io/eslint-plugin-svelte/rules/require-optimized-style-attribute/) | require style attributes that can be optimized | |

## Stylistic Issues
Expand Down
1 change: 1 addition & 0 deletions docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ These rules relate to better ways of doing things to help you avoid problems:
| [svelte/no-reactive-literals](./rules/no-reactive-literals.md) | Don't assign literal values in reactive statements | :bulb: |
| [svelte/no-unused-svelte-ignore](./rules/no-unused-svelte-ignore.md) | disallow unused svelte-ignore comments | :star: |
| [svelte/no-useless-mustaches](./rules/no-useless-mustaches.md) | disallow unnecessary mustache interpolations | :wrench: |
| [svelte/prefer-destructured-store-props](./rules/prefer-destructured-store-props.md) | (no description) | |
| [svelte/require-optimized-style-attribute](./rules/require-optimized-style-attribute.md) | require style attributes that can be optimized | |

## Stylistic Issues
Expand Down
53 changes: 53 additions & 0 deletions docs/rules/prefer-destructured-store-props.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
---
pageClass: "rule-details"
sidebarDepth: 0
title: "svelte/prefer-destructured-store-props"
description: "Destructure store props for more efficient redraws"
---

# svelte/prefer-destructured-store-props

> Destructure store props for more efficient redraws

- :exclamation: <badge text="This rule has not been released yet." vertical="middle" type="error"> **_This rule has not been released yet._** </badge>

## :book: Rule Details

This rule reports on directly accessing properties of a store containing an object. These usages can instead be written as a reactive statement using destructuring to allow for more granular change-tracking and reduced redraws in the component.

An example of the improvements can be see in this [REPL](https://svelte.dev/repl/7de86fea94ff40c48abb82da534dfb89)

<ESLintCodeBlock>

<!--eslint-skip-->

```svelte
<script>
/* eslint svelte/prefer-destructured-store-props: "error" */
$: ({ foo } = $store)
</script>

<!-- ✓ GOOD -->
{foo}

<!-- ✗ BAD -->
{$store.foo}
```

</ESLintCodeBlock>

## :wrench: Options

Nothing

## :heart: Compatibility

This rule was taken from [@tivac/eslint-plugin-svelte].
This rule is compatible with `@tivac/svelte/store-prop-destructuring` rule.

[@tivac/eslint-plugin-svelte]: https://github.com/tivac/eslint-plugin-svelte/

## :mag: Implementation

- [Rule source](https://github.com/ota-meshi/eslint-plugin-svelte/blob/main/src/rules/prefer-destructured-store-props.ts)
- [Test source](https://github.com/ota-meshi/eslint-plugin-svelte/blob/main/tests/src/rules/prefer-destructured-store-props.ts)
130 changes: 130 additions & 0 deletions src/rules/prefer-destructured-store-props.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import type * as ESTree from "estree"
import type { TSESTree } from "@typescript-eslint/types"
import type { AST } from "svelte-eslint-parser"
import { createRule } from "../utils"
import { getStringIfConstant } from "../utils/ast-utils"

type NodeRecord = { property: string; node: TSESTree.MemberExpression }

export default createRule("prefer-destructured-store-props", {
meta: {
docs: {
description:
"Destructure values from object stores for better change tracking & fewer redraws",
Copy link
Contributor

Choose a reason for hiding this comment

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

The first letter should be lowercase to be consistent with existing rules. (see #218)

category: "Best Practices",
recommended: false,
},
hasSuggestions: true,
schema: [],
messages: {
useDestructuring: `Destructure {{property}} from {{store}} for better change tracking & fewer redraws`,
fixUseDestructuring: `Using destructuring like $: ({ {{property}} } = {{store}}); will run faster`,
},
type: "suggestion",
},
create(context) {
let script: AST.SvelteScriptElement

// Store off instances of probably-destructurable statements
const reports: NodeRecord[] = []

// Store off defined variable names so we can avoid offering a suggestion in those cases
const defined: Set<string> = new Set()

return {
[`SvelteScriptElement`](node: AST.SvelteScriptElement) {
script = node
},

// Capture import names
[`ImportSpecifier, ImportDefaultSpecifier, ImportNamespaceSpecifier`](
node: TSESTree.ImportSpecifier,
) {
const { name } = node.local

defined.add(name)
},

// Capture variable names
[`VariableDeclarator[id.type="Identifier"]`](
node: TSESTree.VariableDeclarator,
) {
const { name } = node.id as TSESTree.Identifier

defined.add(name)
},

// {$foo.bar}
// should be
// $: ({ bar } = $foo);
// {bar}
// Same with {$foo["bar"]}
[`MemberExpression[object.name=/^\\$/]`](
node: TSESTree.MemberExpression,
) {
const property =
node.property.type === "Identifier"
? node.property.name
: getStringIfConstant(node.property as ESTree.Expression)

if (!property) {
return
}

reports.push({ property, node })
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the following test case? I tested it and noticed a false positive.

<script>
  $: foo2 = $store.foo;
</script>

{foo2}

Copy link
Member

Choose a reason for hiding this comment

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

Also, could you add the following test case? I think member access in reactive statements should probably be ignored.

<script>
  import store from "./store.js";
	
  let foo, bar
  $: {
    foo = $store.foo;
    bar = $store.bar;
  }
</script>

<div>
  foo: {foo + " " + Date.now()}
</div>
<div>
  bar: {bar + " " + Date.now()}
</div>

Ideally it would be nice to use reactive variables, but I think that could be checked with another new rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you add the following test case? I tested it and noticed a false positive.

<script>
  $: foo2 = $store.foo;
</script>

{foo2}

I don't see the false positive here, can you elaborate?

https://svelte.dev/repl/1d043d248c0c4200b809ca84242301f1?version=3.49.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, could you add the following test case? I think member access in reactive statements should probably be ignored.

I think they should still trigger, but they shouldn't have suggestions. Added tracking of imported & defined names to not suggest if there would be a collision.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you'll see any difference in render times between using destructuring and assigning values via member access.

https://svelte.dev/repl/897da31522c3410596f0dbc4916e5014?version=3.49.0

In my opinion, these member accesses should not be reported as the purpose of this rule is to reduce render times.
In addition to this rule, I think it makes sense to add a new rule that corrects member access to the destructuring writing style. The purpose of the new rules is to enforce stylistic conventions.
In my opinion, it makes sense to have separate rules because they serve different purposes.

What do you think?

Copy link
Member

@ota-meshi ota-meshi Aug 10, 2022

Choose a reason for hiding this comment

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

I just noticed that the REPL you provided in #205 seems to have incorrect results due to typo.

-	<code>direct = obj.a</code><br />{obj.a + " " + Date.now()}	
+	<code>direct = obj.a</code><br />{direct + " " + Date.now()}	

You wrote the following, but when I fix the typo there is no difference in the timestamps.

Notice how often the timestamp is changing on the "obj.a" or "direct = obj.a" versions vs the "({ a } = obj)" version? Each change in that timestamp indicates another redraw for the element.

Copy link
Member

@ota-meshi ota-meshi Aug 10, 2022

Choose a reason for hiding this comment

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

Looking at this result, in my opinion, prefer-reactive-destructuring may also be better split rules.

Copy link
Member

Choose a reason for hiding this comment

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

The purpose of prefer-reactive-destructuring might be seen as enforcing stylistic conventions rather than reducing rendering times.

Copy link
Member

Choose a reason for hiding this comment

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

If we split the rules, it doesn't seem necessary to add a stylistic rule.
The prefer-destructuring rule and the new svelte/prefer-reactive-destructuring rule seem to report them.

<script>
  /* eslint svelte/prefer-reactive-destructuring: error */
  $: foo2 = $store.foo;
</script>
<script>
/* eslint prefer-destructuring: error */

import store from "./store.js";
  
let foo, bar
$: {
  foo = $store.foo;
  bar = $store.bar;
}
</script>

prefer-destructuring DEMO

},

[`Program:exit`]() {
reports.forEach(({ property, node }) => {
const store = (node.object as TSESTree.Identifier).name

context.report({
node,
messageId: "useDestructuring",
data: {
store,
property,
},

// Avoid suggestions for:
// dynamic accesses like {$foo[bar]}
// no <script> tag
// no <script> ending
// variable name already defined
suggest:
node.computed ||
!script ||
!script.endTag ||
defined.has(property)
? []
: [
{
messageId: "fixUseDestructuring",
data: {
store,
property,
},

fix(fixer) {
// This is only necessary to make TS shut up about script.endTag maybe being null
// but since we already checked it above that warning is just wrong
if (!script.endTag) {
return []
}

return [
fixer.insertTextAfterRange(
[script.endTag.range[0], script.endTag.range[0]],
`$: ({ ${property} } = ${store});\n`,
),
fixer.replaceText(node, property),
]
},
},
],
})
})
},
}
},
})
2 changes: 2 additions & 0 deletions src/utils/rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import noUnknownStyleDirectiveProperty from "../rules/no-unknown-style-directive
import noUnusedSvelteIgnore from "../rules/no-unused-svelte-ignore"
import noUselessMustaches from "../rules/no-useless-mustaches"
import preferClassDirective from "../rules/prefer-class-directive"
import preferDestructuredStoreProps from "../rules/prefer-destructured-store-props"
import preferStyleDirective from "../rules/prefer-style-directive"
import requireOptimizedStyleAttribute from "../rules/require-optimized-style-attribute"
import shorthandAttribute from "../rules/shorthand-attribute"
Expand Down Expand Up @@ -59,6 +60,7 @@ export const rules = [
noUnusedSvelteIgnore,
noUselessMustaches,
preferClassDirective,
preferDestructuredStoreProps,
preferStyleDirective,
requireOptimizedStyleAttribute,
shorthandAttribute,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
[
{
"message": "Destructure bar from $foo for better change tracking & fewer redraws",
"line": 6,
"column": 2,
"suggestions": [
{
"desc": "Using destructuring like $: ({ bar } = $foo); will run faster",
"messageId": "fixUseDestructuring",
"output": "<!-- prettier-ignore -->\n<script>\n\n$: ({ bar } = $foo);\n</script>\n\n{bar}\n\n{$foo[baz]}\n\n<!-- eslint-disable-next-line dot-notation -- tests -->\n{$foo[\"qux\"]}\n"
}
]
},
{
"message": "Destructure baz from $foo for better change tracking & fewer redraws",
"line": 8,
"column": 2,
"suggestions": null
},
{
"message": "Destructure qux from $foo for better change tracking & fewer redraws",
"line": 11,
"column": 2,
"suggestions": null
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!-- prettier-ignore -->
<script>

</script>

{$foo.bar}

{$foo[baz]}

<!-- eslint-disable-next-line dot-notation -- tests -->
{$foo["qux"]}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
[
{
"message": "Destructure bar from $foo for better change tracking & fewer redraws",
"line": 2,
"column": 2,
"suggestions": null
},
{
"message": "Destructure bar from $foo for better change tracking & fewer redraws",
"line": 4,
"column": 2,
"suggestions": null
},
{
"message": "Destructure bar from $foo for better change tracking & fewer redraws",
"line": 7,
"column": 2,
"suggestions": null
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!-- prettier-ignore -->
{$foo.bar}

{$foo[bar]}

<!-- eslint-disable-next-line dot-notation -- tests -->
{$foo["bar"]}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
[
{
"message": "Destructure foo from $store for better change tracking & fewer redraws",
"line": 8,
"column": 11,
"suggestions": null
},
{
"message": "Destructure bar from $store for better change tracking & fewer redraws",
"line": 9,
"column": 11,
"suggestions": null
},
{
"message": "Destructure baz from $store for better change tracking & fewer redraws",
"line": 22,
"column": 9,
"suggestions": null
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<!-- prettier-ignore -->
<script>
import store from "./store.js";
import baz from "somewhere"

let foo, bar
$: {
foo = $store.foo;
bar = $store.bar;
}
</script>

<div>
<!-- eslint-disable-next-line prefer-template-->
foo: {foo + " " + Date.now()}
</div>
<div>
<!-- eslint-disable-next-line prefer-template-->
bar: {bar + " " + Date.now()}
</div>
<div>
baz: {$store.baz}
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<!-- prettier-ignore -->
{$foo[`bar${baz}`]}
{foo$.bar}
{f$oo.bar}
16 changes: 16 additions & 0 deletions tests/src/rules/prefer-destructured-store-props.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { RuleTester } from "eslint"
import rule from "../../../src/rules/prefer-destructured-store-props"
import { loadTestCases } from "../../utils/utils"

const tester = new RuleTester({
parserOptions: {
ecmaVersion: 2020,
sourceType: "module",
},
})

tester.run(
"prefer-destructured-store-props",
rule as any,
loadTestCases("prefer-destructured-store-props"),
)