svelte icon indicating copy to clipboard operation
svelte copied to clipboard

$state() of Array : incorrect behavior of indexOf()/findIndex() with object

Open adiguba opened this issue 1 year ago • 9 comments

Describe the bug

When we used an $state([]), some basic function of Array have an incorrect behavior with object. It seems that all equality operations fail, as the object in the array is a proxy.

	let array = $state([]);
	
	const obj = { data: "any value" };
	array.push(obj);
	
	console.log( array[0] === obj );                 // false !!!
	console.log( array.indexOf(obj) );               // -1 !!!
	console.log( array.findIndex(n => n === obj) );  // -1 !!!

It’s really disturbing and not easy to understand...

Reproduction

Exemple in REPL : the buttons addRandomNumber, addRandomString and addRandomObject add a number/string/object in an array, and logs the index.

This works as espected for number and string, but fails for objects.

REPL

Logs

No response

System Info

REPL

Severity

annoyance

adiguba avatar May 11 '24 17:05 adiguba

Maybe use $state.frozen instead? REPL

As I understand it, there's no way to both make an object/array deeply reactive and maintain equality with the original object/array at the same time, since you have to add extra stuff to make it reactive, and you wouldn't want those extra stuff to propagate back to the original object.

longnguyen2004 avatar May 11 '24 18:05 longnguyen2004

I think this is more a documentation problem. Svelte uses proxies to make deep reactivity work, and thus equality of things you put in state will never match the same thing as what you put in, because they're all wrapped with proxies.

trueadm avatar May 11 '24 19:05 trueadm

The workaround seems to reactify the searched value: the $state never creates wrappers (not sure how reliable this behaviour is). Basically:

const value = {};
$state(value) === $state(value); // true
$state(value) === $state($state(value)); // true

const arr = $state([ value ]);
arr.includes($state(value)); // true

REPL

The proxy could alter the behaviour of search methods, but it won't resolve cases like arrState[0] == rawObject.

7nik avatar May 11 '24 19:05 7nik

@dummdidumm I feel like the new warnings largely address this issue now.

trueadm avatar May 14 '24 21:05 trueadm

There needs to be more docs on this, the warning itself is somewhat vague without any additional explanation IMO. It could come as part of the general "more details on a warning" system we can put in place with how warnings are setup now.

dummdidumm avatar May 14 '24 21:05 dummdidumm

There needs to be more docs on this, the warning itself is somewhat vague without any additional explanation IMO. It could come as part of the general "more details on a warning" system we can put in place with how warnings are setup now.

It might be worth documenting now whilst it’s fresh in your head. I don’t personally think it’s all that needed so maybe your documentation can help me understand your thought process.

trueadm avatar May 14 '24 22:05 trueadm

This should be addressed by https://github.com/sveltejs/svelte/pull/11613

longnguyen2004 avatar May 15 '24 08:05 longnguyen2004

This should be addressed by #11613

	console.log( array[0] === obj );                 // false !!!
	console.log( array.indexOf(obj) );               // -1 !!!
	console.log( array.findIndex(n => n === obj) );  // -1 !!!

Here is only for 1st line a solition in Svelte 5. For other two lines - here is nothing, but we can use a for loop.

I'm unhappy about so many extra runes... It looks like React now - solution for self created problems.

dm-de avatar May 15 '24 16:05 dm-de

@dm-de It's not a problem if you reference things in state from the beginning. If you don't then you'll need to use $state.is. This is just how JavaScript proxies work, if you'd rather not use them, and you'd rather use immutable objects instead then you have $state.frozen where you don't need to worry about proxies.

trueadm avatar May 15 '24 16:05 trueadm

#11613 add a partial fix for this, and now these lines will produce a warning state_proxy_equality_mismatch :

items.findIndex(n => n === item);  // WARN state_proxy_equality_mismatch

This is fine, and the warning message is explicit enough :

Reactive $state(...) proxies and the values they proxy have different identities. Because of this, comparisons with === will produce unexpected results. Consider using $state.is(a, b) instead

=> So a === b should be replaced by $state.is(a, b) :

- items.findIndex(n => n === item);
+ items.findIndex(n => $state.is(n, item));

👍

But for indexOf the warning is ambiguous :

items.indexOf(item);  // WARN state_proxy_equality_mismatch

Reactive $state(...) proxies and the values they proxy have different identities. Because of this, comparisons with array.indexOf(...) will produce unexpected results. Consider using $state.is(a, b) instead

=> How do we replace array.indexOf(...) with $state.is(a, b) ???

In fact here we should replace the indexOf() by a findIndex() :

- items.indexOf(item);
+ items.findIndex(n => $state.is(n, item));

So :

  • The message for indexOf() should be more explicit.
  • I still think Svelte should specifically handle calls to indexOf()/lastIndexOf().

In "pseudo-code", the indexOf() of an reactive array should be equivalent to :

indexOf(item) {
   if (item!=null && typeof item === 'object) {
      // item is an object, use findIndex()
      return this.findIndex(n => $state.is(n, item));
   } else {
      return /* original indexOf() */
   }
}

adiguba avatar May 27 '24 17:05 adiguba

I still think Svelte should specifically handle calls to indexOf()/lastIndexOf()

We're getting into monkey-patching territory with this, and I'm pretty sure everyone agrees that we shouldn't do that.

longnguyen2004 avatar May 27 '24 17:05 longnguyen2004

This ticket should not be closed

mustafa-hanif avatar Oct 16 '24 12:10 mustafa-hanif

There is no more $state.is function but the warning is still there.

zdila avatar Oct 22 '24 10:10 zdila

@zdila For more information: https://github.com/sveltejs/svelte/commit/0812b10100f8601f1d342ab5b7bbe9b152f5b15b#diff-60fcfe5f45f9e3aa0d732b8574818764c0bdb326089387090b4cd503413ed3a8

Oliver-Piorun avatar Oct 25 '24 22:10 Oliver-Piorun