feat: add `no-unnecessary-state-wrap` rule

[baseballyama]

close: https://github.com/sveltejs/eslint-plugin-svelte/issues/1052

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: fe121fd87c04d1a8ef36f34a02cdfc7b1f2726ca

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	Minor

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

[github-actions[bot]]

Try the Instant Preview in Online Playground

ESLint Online Playground

Install the Instant Preview to Your Local

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

Published Instant Preview Packages:

• eslint-plugin-svelte: https://pkg.pr.new/eslint-plugin-svelte@1062

View Commit

[marekdedic]

Hi, sorry if this is unfinished and you're planning to add this, but atm, this ignores https://github.com/sveltejs/eslint-plugin-svelte/issues/1052#issuecomment-2630480779

[baseballyama]

Since the clear function can be used, $state is usually unnecessary. If there is a reason to use $state(new SvelteMap()), this rule can simply be disabled. ESLint is fully configurable.

[marekdedic]

I still have to disagree - the rule now has a clear false positive that can be detected by its implementation, but just isn't. And I'm sorry, but yeah, you can disable any rule for any line, but if the FP can be detected by the rule, it should be. Only when it's not possible (like what we're discussing in #910 ) should users be forced to manually disable rules...

[baseballyama]

Can you provide an example of fp?

[marekdedic]

Of course, see https://github.com/sveltejs/eslint-plugin-svelte/issues/1052#issuecomment-2630480779 for a simple example or here for a real-world example (this one is in $derived, but the same semantics apply)

[baseballyama]

Then, let’s add an option to not report an error when a reassignment exists.

[marekdedic]

Thanks :) I don't even think it needs to be an option - if a $state(SvelteMap) is reassigned, the $state is always necessary...

[baseballyama]

I don’t think so. I believe it might simply be a mistake. At least in my project, I want to consistently treat it as an error. For example, in the following case, it can clearly use “clear”.

  let map = $state(new SvelteMap());
  map = new SvelteMap();

[marekdedic]

Hmm, you are absolutely right that the code snippet is incorrect, thanks, I din't realize that!

I am thinking if this isn't a separate issue, though - the anti-pattern here is IMO assigning a new SvelteMap() to an object which is a SvelteMap instead of using .clear(). And that would also be an issue with the regular Map, wouldn't it? So maybe you found a more general issue here that assigning new X() to a variable of type X is incorrect and can be replaced with X.clear() for X being any of SvelteMap, SvelteSet, Map, Set (I checked MDN and that's all I could find)

Now if that were a separate rule it would nicely dovetail with this one even if this rule ignored all re-assigned variables, the other rule would replace the re-assignment with .clear() and this rule would then remove the $state.

I feel I am rambling a little bit - do I make any sense to you? :D

[baseballyama]

In any case, for those who don’t want this rule enabled, they can simply disable it. In most cases, $state(new SvelteMap()) doesn’t really make sense, so I think having it enabled by default isn’t a bad idea.

[baseballyama]

I added allowReassign option.