-
-
Notifications
You must be signed in to change notification settings - Fork 51
feat: add excludedRunes
option to the prefer-const
rule
#1064
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
🦋 Changeset detectedLatest commit: 8ecf189 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Try the Instant Preview in Online PlaygroundInstall the Instant Preview to Your Local
Published Instant Preview Packages:
|
15e0038
to
c3d5bd7
Compare
I still don't understand why you want to ignore I know that some users want to use |
We might want to enable In other words, semantically, since (Personally, I always use const consistently, though.😅) |
In this case, there is no actual reassignment, so 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;
}
}
} |
I understand that when a user uses |
Therefore, I am considering creating a separate rule, |
@ota-meshi So, what you’re saying is that extending |
No, I think Since we don't ignore If a primitive value of |
I still don’t fully understand this part. Since |
If in that example, |
The area I have an issue with is the consistency of the rule. |
I fully understand your perspective.👍 As stated in the documentation, Svelte runes recommend using In your example, since user does not use runes, I understood that standard JavaScript rules apply (without being overridden by Svelte rules). Therefore, I believed that the behavior of this PR was consistent. Additionally, based on your reasoning, shouldn’t |
I think the reason why
Hmm... I don't think it explicitly says "recommended". I think it's still a matter of user preference 🤔 |
I see. Would adding a config be a good middle ground? Like |
If there are users requesting it, I think it makes sense to add the option. |
Got it. In that case, since there are no breaking changes, I will try implementing this option after the v3 release. |
$state
in the prefer-const
ruleexcludedRunes
option to the prefer-const
rule
I added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR introduces a new configuration option, excludedRunes, for the prefer-const rule so that Svelte reactive values (such as $props, $derived, and now $state) can be ignored during const preference analysis. Key changes include updates to test fixtures, documentation, rule implementation, and the rule’s schema to support the new option.
Reviewed Changes
File | Description |
---|---|
packages/eslint-plugin-svelte/tests/fixtures/rules/prefer-const/invalid/option1/test01-errors.yaml | Updates to error fixture properties to include line and column details |
packages/eslint-plugin-svelte/tests/fixtures/rules/prefer-const/invalid/option2/test01-errors.yaml | Updates to error fixture properties to include line and column details |
.changeset/sixty-cars-fail.md | Changeset entry for the new feature |
docs/rules/prefer-const.md | Documentation updated to reflect the addition of the excludedRunes option |
packages/eslint-plugin-svelte/src/rules/prefer-const.ts | Updated rule implementation and schema to accept and use the excludedRunes option |
packages/eslint-plugin-svelte/src/rule-types.ts | Updated type definitions to include the excludedRunes property |
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
docs/rules/prefer-const.md:49
- The configuration property 'ignoreReadonly' in the docs is inconsistent with the rule schema, which defines the property as 'ignoreReadBeforeAssign'. Consider aligning these property names to avoid confusion.
"ignoreReadonly": true,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
close part of #818
As explained here,
$state
also needs to be excluded from theprefer-const
rule.#985 (comment)