no-unnecessary-state-wrap triggers too often
#1154
Development PR
Issue
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
Info
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.
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
@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?
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 where the value is an empty SvelteMap I think that would be even better