no-unnecessary-state-wrap triggers too often

[benmccann]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.

What version of ESLint are you using?

9

What version of eslint-plugin-svelte are you using?

3.3.3

What did you do?

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

What did you expect to happen?

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

What actually happened?

It said that I should not use $state in either location.

Link to GitHub Repo with Minimal Reproducible Example

The full repo is here: https://github.com/immich-app/immich/tree/main/web

I can create a smaller version if needed

Additional comments

No response

[baseballyama]

We can suppress the warning by setting allowReassign to true.
https://sveltejs.github.io/eslint-plugin-svelte/rules/no-unnecessary-state-wrap/#options

I think we can discuss the default value. Personally, I slightly preferred using clear & add to keep the reactive tree smaller.

[benmccann]

Oh, yeah, I think allowReassign defaulting true could potentially make sense.

Is there a case with SvelteSet where using clear and add keeps the reactive tree smaller? I think there could be with custom classes, but I'm having a harder time seeing it with SvelteSet

[marekdedic]

@benmccann I originally thought the same and argued with @baseballyama in #1062 but in the end, I agree that assigning to svelte/reactivity type variables is mostly an anti-pattern. Maybe the docs could be better for this rule, however?

[benmccann]

Catching up on that thread, I agree that map = new SvelteMap(); could be considered an anti-pattern as you can simply do map.clear().

However, in the case that I ran into, the code was selectedAssetIds = new SvelteSet(assets.map((asset) => asset.id));. This does not seem like an anti-pattern to me as it's far more cumbersome to write:

selectedAssetIds.clear();
assets.map((asset) => asset.id).forEach((value) => selectedAssetIds.add(value));

I think false positives are far more harmful than false negatives, so would still lean towards defaulting allowReassign to true. If we could refine that to still not allow reassignments to empty SvelteMap I think that would be even better