Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reactivity problem with objects/arrays in some case (toString) #13054

Closed
adiguba opened this issue Aug 28, 2024 · 5 comments · Fixed by #13542
Closed

Reactivity problem with objects/arrays in some case (toString) #13054

adiguba opened this issue Aug 28, 2024 · 5 comments · Fixed by #13542
Milestone

Comments

@adiguba
Copy link
Contributor

adiguba commented Aug 28, 2024

Describe the bug

If you only use one object in a node's template, the node's text/html is not updated correctly when the object's content changes

Ex:

<script>
    let object = $state( [0] ); // or any object with a toString() function

   // ...
</script>


<!-- This is not reactive -->
<div>{object}</div>
<div>{@html object}</div>

<!-- This is reactive -->
<div>{object.toString()}</div>
<div>{@html object.toString()}</div>

The problem seems to be that set_text() from render.js compares the object reference (which does not change) :

export function set_text(text, value) {
	// @ts-expect-error
	if (value !== (text.__t ??= text.nodeValue)) { // incorrect when value is an object
		// @ts-expect-error
		text.__t = value;
		text.nodeValue = value == null ? '' : value + '';
	}
}

One solution might be to convert the value into string before comparing it :

export function set_text(text, value) {
+        value = value == null ? '' : value + '';      
	// @ts-expect-error
	if (value !== (text.__t ??= text.nodeValue)) {
		// @ts-expect-error
+		text.nodeValue = text.__t = value;
-		text.__t = value;
-		text.nodeValue = value == null ? '' : value + '';
	}
}

Same problem for html() from html.js :

export function html(node, get_value, svg, mathml, skip_warning) {
	var anchor = node;

	var value = '';

	/** @type {Effect | undefined} */
	var effect;

	block(() => {
+		var new_value = get_value();
+		new_value = new_value == null ? '' : new_value + '';      
+		if (value === (value = new_value)) {
-		if (value === (value = get_value())) {
			if (hydrating) {
				hydrate_next();
			}
			return;
		}
        ...

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACqVVTW-CQBD9KxPSGI1G7BWQtIcmPbSn2qSJ9ADrqtviLtkdTAzhvxd2pWCrFuiFMF_vzTxeQmatWUyV5Swzi4c7ajnWfZJYEwsPSRmoPY2RFrESqSRlxlNEsgT9gAcYU4RQyvAAc7hRGCIdLmfvI7eqieiDEqyLWVkIkIiUozObmAjFC0rGN8MRHOsBSoqp5IBbpqa62xTyfKRfePlcp5wgExwYJ5LuKMcaQS9VrALjOdy6JmeWMXDjsU7mAffs-h7uRWmxDgfBSczI5zz7hs71vTqrAEXNCR4RK-pXhJ6tQwj56lhp0h6rJathMqyJ_6oKAWDx8LZwvEjaZYfuHMToRoMNuoM4kiGhbsUzkCYuG2zdUTEzBZKGhTJ7Cg54kZ9VM3nB6reAv4rNBZ7Fb4IHeA1-Wn_wXkyN-RYXNeXvIlpzrjVN13vMVCvpjvu01O78MReEs5NTGz4unp_-sOHdFncx9DHj6WQLZRsDXeVtjHYj-odFz6O0pu9r19_THSn73dnBwCcb9rPxBYifZi7-VjuxYmtGV5aDMqX5e_4F-oUWDOgGAAA=

Logs

No response

System Info

REPL

Severity

annoyance

@trueadm
Copy link
Contributor

trueadm commented Aug 28, 2024

We actually made this change intentionally to move away from this. It turns out that if you have a large collection of data, and you change some state then going through each row and calling toString() internally on each one actually adds a ton of unnecessary overhead. It's actually really uncommon to pass around objects as strings this way too.

So the recommended pattern for this is to actually do toString() to ensure it's reactivity works as planned. Maybe we need to better document this.

@dummdidumm
Copy link
Member

Yeah this has come in several variants now, TLDR we won't change this. But yes, maybe document this somewhere, though not sure where/how without confusing people.

@adiguba
Copy link
Contributor Author

adiguba commented Aug 28, 2024

In fact I had simply used a simple <pre>{myarray}</pre> for debug, and I didn't understand why it wasn't working!

This is very confusing because it will work if we add text in the node :
<pre>{myarray}</pre> <!-- not reactive -->
<pre>values : {myarray}</pre> <!-- reactive -->

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented Nov 26, 2024

📝 Ran ecosystem CI: Open

suite result
sveltekit ❌ failure

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented Nov 26, 2024

📝 Ran ecosystem CI: Open

suite result
sveltekit ❌ failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants