sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.4k stars 4.1k forks source link

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

Open adiguba opened 2 weeks ago

adiguba commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 -->