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

feat(prefer-let): add rule #985

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

mikededo
Copy link
Contributor

Implements the prefer-let rule.

Closes #818.

Copy link

changeset-bot bot commented Dec 31, 2024

🦋 Changeset detected

Latest commit: ae7f479

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-svelte Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented Dec 31, 2024

Open in Stackblitz

npm i https://pkg.pr.new/eslint-plugin-svelte@985

commit: ae7f479

Copy link
Member

@baseballyama baseballyama left a comment

Choose a reason for hiding this comment

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

It's better to consider $bindable.

@mikededo
Copy link
Contributor Author

It's better to consider $bindable.

Could you explain a bit more in depth? Do you mean only to suggest using let for $props whenever the $bindable is used? Since $props is also modified by Svelte, for those who prefer using let, I thought it made more sense with the initial idea.

@baseballyama
Copy link
Member

I personally prefer using let only when there is a bindable prop, and using const otherwise. It would be better if there are a configuration that allows let only when a bindable prop is present. While writing this, I also thought that prefer-const might need some related adjustments as well.

@baseballyama
Copy link
Member

baseballyama commented Jan 1, 2025

I post more details about this.

$state

Users may want to define $state as either const or let.
When handling objects in $state, reactive updates also work by directly reassigning individual properties.
In such cases, some users might prefer defining it as const.

export const Time3 = () => {
  // If `$state` has object value,
  // Time3 user cn reassign `value.value` even without reassign logic in this composable.
  // Therefore it can be declare as both `const` / `let`
  const value = $state( { value: new Date().toString() });
  
  return {
    get value() {
      return value;
    }
  }
}

$props

The use of const or let for $props depends on whether $bindable is present.

  • Without $bindable, it’s a matter of preference.
  • With $bindable, it must be declared using let. However, this can be enforced at the language level, not necessarily by ESLint.

$derived

For $derived, choosing const or let is generally a matter of preference.


Conclusion

Given these considerations, this rule might be better named prefer-runes-declare-kind instead of prefer-let.


Detail code for each case is here.

https://svelte.dev/playground/hello-world?version=5.16.0#H4sIAAAAAAAAE6VVwY6jOBD9lRJqqZNdFDrJjSZIPbOXua20s9rDMhIGKsEzxEZ2QXcL8e8rbJMQOpnpnTkBVa_K5vnVc-cJdkQv9HIpNEEAFZLne3teofbCfzuPXushPQQ8fwQ_1fVKt1jREMuYxmvxXApCQdoLvUjnitcUJyIhfqylIviHUykb-sBFwbIKYa_kEe5XwSzu2t0_zkqv1n2vqIPP_Ihr3zw29rGF_lT_UR5rqSfVq6_aNBAJWW5oqIcdCHy2vRbLx8vsBna2_ZvM1mW2NiMS0kifBKFqWbVYLGEXQzeEEzLLrFpWNegW-4MRLpYrkn-R4uLgmlvk5t3IrUW-A9_7sH54eBh3GgTwVD2zVw13mhghlKxFIAkZQlohpQOoQoJcNoJg52AL22AoT-8KVLzFIoWcCSgwr5hCYBoySaVtAkwUkBrG0jN3hWyyCouhqWuxsKv8BpuhfRSclSWiuaY6g-0hiF3ylMm4KEKT3U1BiYgK3oKm1wp3iZdJVaAKYV2_gJYVL-CgEMVj4sWJAIjKdfxU11FQru13Hbv9htC5tz4KapvMGiIpQIq84vm3XeJ19tjt__y-g3WfePHH4evvOgos3ExMVMdGFCF0E3G4zqf0xqU319Nbl76QgQNFQcHb2PM9whfyQlIN9v6N0b8-n5c2cBNzwxKMRGola_1OgQDfA5WoELgGISG9y9xaE-10jtp-UI_p7obvUjTfP3CFxfS4Z382PXqno59j9Id0_hSXupRNVZg5vcLbjLRhgrvzDI-5xfLXCMyqBucM_pA-gIiLuiE7p0aq45z-T3Kv2foluzcQE3rxxdwgecW0ts5vvToIwJppIVGLe7LGqJBpzQ8CsoaASq5dYcsUH1bxQctL8bqzCUY3BRg92jnpVasejqIfDmDcnhH9Z3cNTa6UIIA_FT9y4i1CaluetHG55UoeeG42yKzjc5qKaGr2793igFdIjRLjBXcYqxfLMXSCmLi7tHrfPvUJ304KxvXbEW1urluUbN9Q8ml_5qJkGmT2FXO3kO8wtrLRqCAXZ5bSiYOmgC0KeLa-MGMSuHAKOGtsbD0IYC8HEZDxvAzf2t5cGKOzzbiHzkbC63c69L94DiOzlxP3pf8PGT3MiTwKAAA=

@baseballyama
Copy link
Member

And for the prefer-const rule, it seems best to simply ignore runes.
(#933 doesn’t handle $state, but as mentioned above, there may be cases without reassignment. So, it should be treated like $props.)

@mikededo mikededo marked this pull request as draft January 2, 2025 15:24
@mikededo
Copy link
Contributor Author

mikededo commented Jan 2, 2025

@baseballyama I see what you mean. The original discussion started because rune values may not be reassigned in the code itself, but they are by Svelte underthehood: #818 (comment).

While I'm more on the second view (to declare everything as const unless modified in the code), I implemented the rule taking into account the first view, which is what I understood was better (#818 (comment)), based on the discussion.

At the same time, maybe it'd make more sense to have one rule, which would force either let or const based on certain conditions or preference. For instance, enforce const on $derived; let on $props whenever there's a $bindable; let on $props (as it they can be modified by Svelte)...

@bfanger
Copy link

bfanger commented Jan 3, 2025

Duplicate of #806 or was my implementation missing something?

The references from both $derived & $props are reassigned by Svelte.

const as defined in the Javascript spec is adding a check at runtime that the the reference cannot be reassigned.
A reassignment should throw an Uncaught TypeError: Assignment to constant variable.
But because signals are compiled into objects and you can reassign properties even when an object is a const (the signal.v property) javascript is unable to detect this error.

So if we want one rule (or a default configuration), I suggest following the Javascript specification and use let for both $props & $derived.

This is also the way all examples are written in the the Svelte documentation.

@baseballyama
Copy link
Member

I want to enforce const for props instead of let to maintain data flow integrity, even if the compiler reassigns values under the hood. (I am speaking with the assumption that I have a relatively good understanding of the Svelte 5 runtime.)

@mikededo
Copy link
Contributor Author

mikededo commented Jan 3, 2025

Duplicate of #806 or was my implementation missing something?

No! Not really, just that it had not been updated for 7 months, and I thought I'd be a nice addition to the plugin.

At the same time, I'm starting to prefer a rule that allows users to choose whether they prefer let or const for Svelte reactive assignments, over two different rules.
What do you think about keeping the prefer-const rule, as it does make sense overwriting the base rule, and working on a second rule to customise what to report for reactive assignments?

@mikededo
Copy link
Contributor Author

mikededo commented Jan 3, 2025

I want to enforce const for props instead of let to maintain data flow integrity, even if the compiler reassigns values under the hood. (I am speaking with the assumption that I have a relatively good understanding of the Svelte 5 runtime.)

I agree with this, but I also understand those who prefer let. Since this is truly an opinionated decision, I'd rather provide a customisable rule, than one for each case.
This way, we avoid bloating prefer-const and/or prefer-let with customisations, and unify them in one single rule that handles all cases.

@baseballyama
Copy link
Member

What do you think about keeping the prefer-const rule, as it does make sense overwriting the base rule, and working on a second rule to customise what to report for reactive assignments?

Yes. I think current prefer-const rule is nice. (But we need to ignore $state also)
And adding prefer-runes-declare-kind is also sounds good instead of prefer-let.

@bfanger
Copy link

bfanger commented Jan 3, 2025

We could make "svelte/prefer-const" configurable to enforce the described let or const behavior:

rules: {
  "svelte/prefer-const": { signals: "let" }
}

Where signals can be configured to "let", "const" or "ignore".

let: Show a linting error when using const for signals. Autofix to let
const: Show linting error when using let, but not reassigned in the file or uses $bindable. Autofix to const where possible
ignore: No linting error, keep the let or const as-is

@mikededo
Copy link
Contributor Author

mikededo commented Jan 3, 2025

I'd rather implement @baseballyama's proposal, as adding for instance, I may want const for $props unless there's a $bindable prop.

We could re-write the prefer-const so that it also overwrites the base eslint/prefer-const behaviour.

@bfanger
Copy link

bfanger commented Jan 3, 2025

@mikededo That should be the behavior of:

rules: {
  "svelte/prefer-const": { signals: "const" }
}

@baseballyama
Copy link
Member

Adding an option to prefer-const also sounds like a good idea.

@baseballyama
Copy link
Member

baseballyama commented Jan 3, 2025

In any case, it seems that there is a consensus regarding the requirements that should be implemented as rules.

I think adding an option to the prefer-const rule is a good approach. However, I’m concerned about the potential difficulty of keeping up with future changes to the prefer-const rule in original ESLint. (I’d prefer to avoid copying and re-implementing the code of the original ESLint rule.)

In that regard, it might be safer to limit changes to the prefer-const rule to simply ignoring the runes variable and instead create a separate rule that specifies how const, let, or var should be declared for runes. This approach seems to pose less risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rules: rune-prefer-let & prefer-const
3 participants