svelte/prefer-svelte-reactivity triggers inside non-reactive functions (e.g., URL, Date, Set usage)

[cnolte]

Version: eslint-plugin-svelte@3.11.0

The svelte/prefer-svelte-reactivity rule currently triggers even inside regular functions where Svelte's reactivity is not needed or doesn't make sense.

e.g.:

const getNextPage = () => {
  const newUrl = new URL(page.url) // !!! This leads to an error
  newUrl.pathname = page.url.pathname
  newUrl.searchParams.set('p', (pageNum + 1).toString())
  return newUrl.toString()
}

In this context:

• new URL(...) is a straightforward operation.
• There's no reactive dependency tracking needed.
• Restructuring this logic into a reactive statement (e.g., using SvelteURL()) would be unnecessary.
• I would also expect the reactive version to be slower than the native one.

I've encountered similar false positives with other built-ins like Date and Set.

[marekdedic]

Hi, thanks for the report, I am ready to fix any issues coming from the new rule :) However, I don't understand your example yet, so I need your help to really understand it before fixing it.

The rule currently triggers on any non-reactive class (e.g. URL) that fulfills at leas one of the following conditions:

• It is modified in some way (which is different for each class, but in your example assigning to newUrl.pathname counts as a modification)
• It is exported out of a .svelte.js file

In your example, the newUrl object is modified, so it is reported. And I'm not really sure why this is an FP - something like this seems more idiomatic to Svelte:

currentUrl = new SvelteURL(page.url);
currentUrl.pathname = page.url.pathname;

function getNextPage() {
    currentUrl.searchParams.set('p', (pageNum + 1).toString())
}

// Later, somewhere...
const pageData = $derived(fetch(currentUrl.toString()));

Am I missing something? So far it seems to me that the rule pushes you towards a more idiomatic style, which is IMO a good thing...

[cnolte]

Thanks for the explanation. Your rewrite changes the logic because I want to render the url for the next page. But you're right that it's not very clean. I rewrote the function so that it only depends on the input parameters. In this case, I don't need to use newUrl.pathname = page.url.pathname. This still feels pretty limiting but ok :-)

[marekdedic]

Ok, great. But do let me know if you find anything that is a clear FP, if there's some way to tell with eslint rules (which is sometimes hard), I would like to make the rule as usable as possible :)

[marklbarlow]

I think it this is still massively reporting false positives unfortunately and should be re-opened. I'm seeing two repeating examples currently in .svelte.ts files.

Private member variables inside a class that are used across various methods. However, none of these methods are reactive.

export class Example {
  // This triggers a false positive
  #mySet = new Set<string>();
     
  add(value: string) {
    this.#mySet.add(value);
  }
  
  has(value: string) {
    return this.#mySet.has(value);
  }
  
  delete(value: string) {
    this.#mySet.delete(value);
  }
}

Variables inside a $derived block that are entirely locally scoped.

export class Example2 {
  #names = $state<string[]>([]);
  
  add(value: string) {
    this.#names.push(value);
  }
  
  unique = $derived.by(() => {
    // This triggers a false positive
    // (yes, it could be done in 1 line without triggering the rule, but that's not the point)
    const mySet = new Set<string>();

    for (const name of this.#names) {
      mySet.add(name);
    }
    
    return mySet;
  }

Neither of the above scenarios should trigger the rule.

In principle, the rule is a great idea but as currently implemented it's unfortunately unusable.

[dimfeld]

Adding on to the above, I'm seeing it trigger even in Sets that exist only within a single function and don't escape the scope, meaning that any function that uses a Set or Map internally to save or reuse computation will flag the lint.

For example, this is enough to trigger the lint rule:

 function abc() {
    let x = new Set([1, 2, 3]);
    x.add(4);
 }

[gyzerok]

For us it also gives lots of false positives, it seems. I would suggest to keep this rule disabled by default for now.

[marekdedic]

Ok, thanks for all the examples! Let's try to dissect and solve them if possible. I see basically the same pattern of a variable being encapsulated without "leaking" outside of the encapsulation:

1. A local variable that is not returned from a function
2. A private class property that isn't returned by any of its methods
3. A local variable in a $derived.by expression (which is actually just case 1., because there is a function in the $derived.by...)

I think it is feasible to add this as an exception to the rule, maybe gated behind a config option. But I am still a little bit uncertain as to why you would want to do that and if the rule isn't actually pushing you towards better code... If I take the example of a function that has a local non-reactive variable:

1. Either the function is entirely non-reactive, in which case it maybe doesn't belong in a Svelte file? (Debatable)
2. Or the function is "called" in a reactive loop, in which case it would be better rewritten as a series of reactive variables, because you potentially get more fine-grained reactivity doing it that way...

Can you provide some real-world examples that are neither of these 2 cases? In the least-toy-like example with the $derived.by by @marklbarlow , it seems like the only reason you create the Set is because you put the elements into an array in the first place...

[kyle-leonhard]

@marekdedic - here's a non-toy (😄) example of a false positive that I stumbled on. The lint triggered on the usage of Set.

let toys = $state([]);

export function getUniqueToys() {
  let names: Set<string> = new Set();
  let uniqueToys = [];
  for (const toy in toys) {
    if (!names.has(toy.name)) {
      uniqueToys.push(toy);
    }
  }
  return uniqueToys;
}

let uniqueToys = $derived.by(getUniqueToys);

[marekdedic]

Hmm, I've been thinking about it for a few days and it would make sense to add a config option to not report "encapsulated" uses. So far there are the 2 cases: function not returning the variable and a class not exposing its private property.

Let me know if you have any other examples and thank you for the non-toy example, it helped me thinking about it. I'll be away for 2 weeks now and I'll take a look at it afterwards.