sveltejs / eslint-plugin-svelte

ESLint plugin for Svelte using AST
https://sveltejs.github.io/eslint-plugin-svelte/
MIT License
307 stars 37 forks source link

New Rules: rune-prefer-let & prefer-const #818

Open bfanger opened 4 months ago

bfanger commented 4 months ago

Motivation

const in javascript:

The const declaration creates an immutable reference to a value. It does not mean the value it holds is immutable — just that the variable identifier cannot be reassigned. You should understand const declarations as "create a variable whose identity remains constant", not "whose value remains constant" — or, "create immutable bindings", not "immutable values".

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const

At the moment the Svelte 5 compiler breaks this rule and we can no longer make assumptions of what should be possible.

<script>
const { value } = $props() 

const copy = value;

console.log(value) // output: 1

function onclick() {
    console.log(value) // output: 3 (prop was updated)
    if (copy === value) { // false ?!?
    }
}
</script>

<button {onclick}>Click me</button>

Because the compiler replaces all usages of reactive values by getters & setters, this doesn't result in aTypeError: Assignment to constant variable. error like it would in regular javascript.

Description

The svelte/rune-prefer-let rule is aware of which variables are reactive by detecting switch are assigned via a rune.

eslint prefer-const rule doesn't see any reassignments so it assumes a let statement can be safely converted to const which conflicts with the svelte/rune-prefer-let

The svelte/prefer-const rule is identical, but takes into account which variable are reactive an can be reassigned.

Examples


<!-- ✓ GOOD -->
<script>
let {count,  onclick} = $props();

let double = $derived(count * 2);
</script>

<!-- ✗ BAD -->
<script>
const {count,  onclick} = $props();

const double = $derived(count * 2);
</script>

Additional comments

Implementation of these rules are available https://github.com/sveltejs/eslint-plugin-svelte/pull/806 & https://github.com/sveltejs/eslint-plugin-svelte/pull/816

ota-meshi commented 2 months ago

The code doesn't change the variables, so I think it's fine to use const. Is it really necessary to use let? :thinking: They work in runes, so it makes sense that they don't match the description on MDN.

I prefer to use const since it is necessary to completely prohibit reassignment of variables using $derived.

mrh1997 commented 2 months ago

@ota-meshi : Although I absolutely can follow your argumentation please keep in mind that this is not what people will intuitively do.

Thus if you stick to your approach all tutorials should be adopted and eslint should at least output a message which tells the user to use "const" when working with "$derived".

ota-meshi commented 2 months ago

I don't force my preferred code style on other people's projects.

mrh1997 commented 2 months ago

I don't think this issue is a question of code style. Changing code style will never break code. But switching between "let" and "const" does (or at least make the linter not accept the code any more).

What I meant: whatever decision the svelte core team mets: please keep in mind to stay consistent here...

bfanger commented 2 months ago

The code doesn't change the variables

The variable is reassigned by code, but not by your code. Something that should be impossible at runtime for const.

I prefer to use const since it is necessary to completely prohibit reassignment of variables using $derived.

It is not necessary. Using either let or const the Svelte compiler will not allow you to reassign a $derived variable. You'll get a Cannot assign to derived state error in your editor/build.

so I think it's fine to use const. Is it really necessary to use let? 🤔 They work in runes, so it makes sense that they don't match the description on MDN.

In a way, const is not necessary either.

When replacing all const statements with let in a codebase, the code will "work" but these variables no longer throw runtime errors when reassigned. Replacing all let variables that are not reassigned will also "work".

But I prefer to use const for variables that are not reassigned.

I don't force my preferred code style on other people's projects.

That the job of the author of a ESLint config, but as a ESLint plugin maintainer you play a rule in forcing a preferred code style.

There are two views:

  1. let means "this variable can be reassigned" const to mean "this variable cannot be reassigned" not by me, not by anyone.

  2. let means "this variable can be reassigned" const to mean "this variable can reassigned by Svelte but not by me".

I am in 1, I want const to mean the same as it does the rest of JavaScript.

Some are in 2 and prefer to use const for $derived because for them it better communicates that this is a readonly value, not necessarily that the value itself never changes and they want to use const for components that only have readonly props.

The issue is that regular ESLint only caters to view 2, creates incorrect code for view 1. Rules like prefer-const are not aware of Svelte's transformations to the code. This is what svelte/prefer-const solves.

ota-meshi commented 2 months ago

There are two views:

Thank you for explaining, I hadn't thought of it that way.

I think it would be a good idea to add rules for users who prefer 1.. Perhaps those users would want to define variables that are not reassigned other than $derived() and $props() as const, and the rest as let, so I think it might be convenient to provide it in a single rule. What do you think? Also, the documentation for those rules should include a note that they conflict with prefer-const.

MathiasWP commented 1 month ago

I'm having the same problem in a component with bindable properties.

If i set const { ... } = $props() then my code triggers a Cannot assign to X because it is a constant. on the bindable properties, and if i change it to let { ... } = $props() then the ESLint prefer-const rule is triggered :/

mikededo commented 1 day ago

Hey @MathiasWP and @ota-meshi, I'll be continuing @bfanger's work in another PR. I think this rule would be helpful in many cases, despite being opinionated and Svelte not strictly defining how it should be done.

mikededo commented 1 day ago

After having worked on this for a bit, how do you feel about adding another property to the rule that would allow devs to prevent reactive declarations from Svelte to be checked by this rule?

For instance, I like having my derived and props as const, therefore I'd only want state and it's variants to be checked. Currently the rule as implemented in #933, it checks for all reactive assignments. So I'm thinking something like:

// eslint.config.js

export default [
  {
    rules: {
      'svelte/prefer-const': {
        ignoreValues: ['state']
      }
    }
  }
];

So:

<script>
  // Good, checked and no error
  const {} = $props();

  // Nothing, rule does not take state into account
  const a = $state();
  let b = $state();

  // Bad, checked and error
  let c = $derived(b);
</script>

Just throwing this as an idea, which would allow more granular configuration.

mikededo commented 17 hours ago

Also, the rule should also work for *.svelte.{js,ts} files, right?