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

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

Open
bfanger opened this issue Jul 6, 2024 · 10 comments
Open

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

bfanger opened this issue Jul 6, 2024 · 10 comments
Labels
enhancement New feature or request new rule v3

Comments

@bfanger
Copy link

bfanger commented Jul 6, 2024

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 #806 & #816

@bfanger bfanger added enhancement New feature or request new rule labels Jul 6, 2024
@ota-meshi
Copy link
Member

ota-meshi commented Sep 13, 2024

The code doesn't change the variables, 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.

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

@mrh1997
Copy link

mrh1997 commented Sep 13, 2024

@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
Copy link
Member

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

@mrh1997
Copy link

mrh1997 commented Sep 13, 2024

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
Copy link
Author

bfanger commented Sep 15, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new rule v3
Projects
None yet
Development

No branches or pull requests

6 participants