feat(no-unnecessary-state-wrap): support string array in allowReassign
, default to true
and improve messages
9
eslint-plugin-svelte
are you using?3.3.3
Tried to upgrade https://github.com/immich-app/immich/tree/main/web to 3.3.3
It triggered no-unnecessary-state-wrap
in two places. One was helpful, but the other I consider to be a false positive
This shouldn't be $state
and should be a const
: https://github.com/immich-app/immich/blob/392ce7deb2683d2c66a821c617329b3641491016/web/src/lib/components/forms/tag-asset-form.svelte#L21
However, this one is pretty reasonable because you're creating a new SvelteSet
and it's a lot more cumbersome to clear
the set and then add
each item: https://github.com/immich-app/immich/blob/90f21d9047aa33dcb8231ab61269e60a32aedd9f/web/src/lib/components/utilities-page/duplicates/duplicates-compare-control.svelte#L86
I think we should trigger the rule only when the variable is not being reassigned
It said that I should not use $state
in either location.
The full repo is here: https://github.com/immich-app/immich/tree/main/web
I can create a smaller version if needed
No response
9.23.0
eslint-plugin-svelte
are you using?3.4.1
In the following code snippet if I remove $state
rune then I get svelte(non_reactive_update)
error, however if I add it I get svelte/no-unnecessary-state-wrap
.
export default ts.config(
includeIgnoreFile(gitignorePath),
js.configs.recommended,
...ts.configs.recommended,
...svelte.configs.recommended,
prettier,
...svelte.configs.prettier,
{
languageOptions: {
globals: { ...globals.browser, ...globals.node }
},
rules: { 'no-undef': 'off' }
},
{
files: ['**/*.svelte', '**/*.svelte.ts', '**/*.svelte.js'],
ignores: ['eslint.config.js', 'svelte.config.js'],
languageOptions: {
parserOptions: {
projectService: true,
extraFileExtensions: ['.svelte'],
parser: ts.parser,
svelteConfig
}
}
}
);
<script lang="ts">
import { SvelteSet } from 'svelte/reactivity';
import Bug3 from './Bug3.svelte';
let set = new SvelteSet<number>([]);
$effect(() => {
console.log(set);
});
</script>
<Bug3 bind:set />
Not entirely sure
Either of these errors
❯ npm run lint
> svelte-eslint-bug@0.0.1 lint
> prettier --check . && eslint .
Checking formatting...
All matched files use Prettier code style!
svelte-eslint-bug/src/routes/+page.svelte
5:19 error SvelteSet is already reactive, $state wrapping is unnecessary svelte/no-unnecessary-state-wrap
✖ 1 problem (1 error, 0 warnings)
❯ npm run check
> svelte-eslint-bug@0.0.1 check
> svelte-kit sync && svelte-check --tsconfig ./tsconfig.json
====================================
Loading svelte-check in workspace: svelte-eslint-bug
Getting Svelte diagnostics...
/Users/f.nezhivoi/Code/work/svelte-eslint-bug/src/routes/+page.svelte:5:6
Warn: `set` is updated, but is not declared with `$state(...)`. Changing its value will not correctly trigger updates
https://svelte.dev/e/non_reactive_update (svelte)
let set = new SvelteSet<number>([]);
====================================
svelte-check found 0 errors and 1 warning in 1 fil
``
### Link to **GitHub Repo** with Minimal Reproducible Example
Updated my repo with this issue: https://github.com/gyzerok/svelte-eslint-bug
### Additional comments
_No response_
Latest commit: 9070e12
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
npm i https://pkg.pr.new/eslint-plugin-svelte@9070e12
I think set = new SvelteSet([1, 2, 3])
should be allowed as it's cumbersome to write another way, but set = new SvelteSet()
should not be because set.clear();
is easier to write
I think set = new SvelteSet([1, 2, 3]) should be allowed as it's cumbersome to write another way, but set = new SvelteSet() should not be because set.clear(); is easier to write
Since the rule would become more complex, would it be better to implement it as a separate rule, like prefer-svelteset-new
?
Why should SvelteSet
be allowed by default? I don't see why it's different from e.g. SvelteMap
, am I missing something?
Since the rule would become more complex, would it be better to implement it as a separate rule, like prefer-svelteset-new?
I think that would be fine. The name seems backwards to me though. Perhaps something like prefer-clear-over-reassignment
. I also think you don't necessarily need svelteset
in the name because it could apply to SvelteMap
as well
Why should SvelteSet be allowed by default? I don't see why it's different from e.g. SvelteMap, am I missing something?
I agree. I think we ended up deciding all reassignments should be allowed by default though, so probably not an issue once that change is made. The prefer-clear-over-reassignment
rule can handle the SvelteSet
and SvelteMap
cases
Thanks 🙏
I'm a bit lost and want to understand this: Am I correct that what this PR essentially does is by default turn off the rule for any SvelteSet or SvelteMap that is re-assigned anywhere?
And then the suggestion is to add a new rule that basically reverts this change?
I'm a bit lost and want to understand this: Am I correct that what this PR essentially does is by default turn off the rule for any SvelteSet or SvelteMap that is re-assigned anywhere?
The purpose of this PR is to change the default value of allowReassign
to true
, and to exclude any reassigned reactive classes including SvelteSet
and SvelteMap
from being reported.
I’ve updated the PR title, and I’ll update the PR this weekend.