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

UI not re-rendering when rune is updated #9301

Closed
peterreeves opened this issue Oct 9, 2023 · 21 comments
Closed

UI not re-rendering when rune is updated #9301

peterreeves opened this issue Oct 9, 2023 · 21 comments
Labels

Comments

@peterreeves
Copy link

peterreeves commented Oct 9, 2023

Describe the bug

So I'm not 100% sure if this is a bug with runes, but it sure feels like one.

In the example below I have a $state rune in a Svelte component, and a $state rune in a JS file. Both are arrays, and I'm pushing values into the array and doing the old list = list trick to trigger an update. This works in the component, but not in the one imported from the JS file. The problem goes away if I uses a spread operator (list = [...list]), but I'd rather reuse the same array to reduce GC pressure (And I also have a considerable amount of code that mutates arrays rather than reallocating. It would be a pain to change it all).

Additionally, is my other export from the JS file 'idiomatic' for global state using runes? Is there a more rune-idiomatic way of writing what I have?

Reproduction

Edit: updated other.js -> other.svelte.js

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACp2SsW7bMBCGX-XAFrAMGNLuyCqCdMmQMehgeVCks8WY4gkkFTcQNOYNOhbo0ifrE-QRehQVx0DcBsgiisf_7n4ev15spUIrlute6KJBsRSXbSsWwj22fmMfUDnkvaXOlD6S2tLI1mW5zp1sWjIOeiBXo4EBtoYayEWcjIE4ZMf3NhcXrOcMhQ6UtA5W8Nm6wmG0zkUtc7GZT4qSNB-3na1ZE81hlUHv45zLebE_iELKmDHFWeuXKeJrkMJY0S7y4aAc-Jsmr_Z1etc5RxpIL0sly_2q99WH7LKqYFbLGTiCkviOGvVkO7ryQrjBeZqEbK6U2rbQ2Tcyewt_fj5xD7_nOKlxTP0nLMo6FCgsr0OwmSqZ9bxLE_7hgJcmXjp4oyH5jMkw21OrqBSNbsM7vOf0K6HVMwcHdvzl-dfvH_9yHDp9xDcT01AltxIrsXSmw2FxBOyUjVfS7u0pZec58fc8ooLfR_gCL-Hiq4DK7iWZ8enBoOuMDnjAsPCCEaL5Wa5CiwAMwFu0kgRuNUPRMBJS74D7wh0qOrBII3RtxWbtGL69XgBPHq550J2qWLBH_0gV8bG0cJCuJj4vFemXUoUxxeOx09R-HcfjI2z-S3euPeBvB78Z_gJmEFQX5QMAAA==

Logs

No response

System Info

N/A

Severity

blocking an upgrade

@peterreeves
Copy link
Author

A kind user on the Svelte discord did some quick analysis of the compiler output:
https://discord.com/channels/457912077277855764/1153350350158450758/1160801864414150676

@peterreeves
Copy link
Author

That's neat, and I'm surprised that works? (makes it definitely feel like current behaviour is a bug). But I need shared exported app-wide state so that doesn't work for me.

@RubenPX
Copy link

RubenPX commented Oct 9, 2023

Seems to be a bug? maybe in future will be fixed

@7nik
Copy link

7nik commented Oct 9, 2023

It was slightly mentioned in another issue that runes are bound to component lifecycle. So the problem isn't that the list is global but rather that list is created before the component. If you switch to singleton, it starts to work https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA51Ty27bMBD8lQVbwBJgSHdHlhG015xziHJQZNqiQ3MFcpUHBB3zBz0W6KVf1i_oJ3RJSnHSJCjQE7XDmeUsORrETmnpxOpqEKY-SrES510nloIeO1-4O6lJcu2wt41HCtdY1VFZmYrUsUNLMABSKy2MsLN4hEpkeQCyg6vEGROZqiWBVo5gDZ8d1SSTq0q0qhLX6cRo0PB217uWOUkK6xIGj7OWdZnfSKIkKCacuX6ZEN8Dtcw07hMPR-b46gS8ObAoGEz8fpGfJjLFTU-EBtCsGq2a2_Xgzx3L8-0WFq1aACE0yGMbaaaBki-eCBcyLfKo5k6F62pTXqK9dfDr-xOf4WvGUYebGz7Jumljg9rxOsYBCq3Kgasi5w8GPDX31NEbjeJ3TPJM2UujUmsMXuPD_MvnV5TOLAju2e_m94-f3z7y68_5H88coCNu1U7JrViR7eW4fM5bsHhK3MG9TFt8sl3dENrH17l4P1F-8jlUZCX11swx2s8CbjLAtBfCA-MyUkLI0lnwV_Ji6zl8AG_j91EAOYJzDmMWvXenzJ5XNIzIh_AjTQENbzbP-kyDzQaSU7WebyVJ07O393s9_gEOrYRR2wMAAA==

Something should be done about this, otherwise this small nuance will drive people crazy.

@RubenPX
Copy link

RubenPX commented Oct 9, 2023

That's awesome i will take this as note for future to manage global states

@7nik i don't think this is a problem. It makes people investigate some coding patterns... I imagine more later it will exist a video, issue or forum that mentionate it, Svelte has extensive docs and a lovely community

@Thiagolino8
Copy link

So the problem isn't that the list is global but rather that list is created before the component

If that were the problem, wouldn't this example also break?

@7nik
Copy link

7nik commented Oct 9, 2023

let's investigate deeper: as was mentioned on discord, runes use different methods of comparing the new and old values, which are set once at their creation:

    const context = current_component_context;
    if (context && !context.immutable) {
      return safe_equal;
    }
    return default_equals;

Where safe_equal uses Svelte3/4 value change logic (objects always get marked as mutated) and default_equals is just ===. Global variables are initialized before components, so current_component_context is null and thus default_equals is used.

Numbers are primitive, and their change gets detected correctly by both methods. But mutation of object/array isn't detected at all. Instead, there is still gets added a trigger on assignment, and if safe_equal is used, the object always gets marked as mutated.

So, the issue narrows down to the reactivity of mutation on a global object created not during a component initialization. (and probably some other lifecycle events).

@peterreeves
Copy link
Author

So I definitely understand that default_equals is probably the right default for most use cases. I'm wondering if it would be possible to have a markMutated function (or equivalent) that could be used to mark a specific object as having been changed? I'd happily write code where I mark out all the places I'm mutating state. I do it all the time anyway with obj = obj; in component bodies, and myStore.update((s) => s); for global stores.

@intrnl
Copy link

intrnl commented Oct 12, 2023

ideally $state should allow passing an equality function, or at least false or true to indicate opting-in and out of the strict equality comparison (with undefined as a way to opt-in to whatever the component is currently set up as)

@stalkerg
Copy link
Contributor

stalkerg commented Oct 18, 2023

@7nik maybe we should back to the idea with equalentors? heh #1657 (comment)

@7nik
Copy link

7nik commented Oct 18, 2023

But the issue is with the value mutation: a comparator (equalentor) will receive in a and b the same object instance.

The logic of choosing between safe_equal and default_equals should be improved (maybe analyze the used types, though it can be a too complicated approach) or provide the user with the ability to select the comparator.

@intrnl
Copy link

intrnl commented Oct 18, 2023

given that the actual function used to create these runes accepts an equality function as the second argument I would've expected $state to allow it, but I suppose they're not certain on the API just yet.

@Thiagolino8
Copy link

A new comparison method is definitely needed
Recreating objects and arrays to achieve reactivity can be quite unpleasant
But having to do this with sets and maps completely defeats the point of using these objects
And having to manually add an equalization function every time you need to use one of these objects is also not ideal

@ryoppippi
Copy link

ryoppippi commented Nov 16, 2023

@Thiagolino8
I fixed your example for svelte@next here
However it does not work
If it is not a Map but an array, it works fine

@Thiagolino8
Copy link

@ryoppippi

If it is not a Map but an array, it works fine

It doesn't work

@ryoppippi
Copy link

ryoppippi commented Nov 16, 2023

@Thiagolino8
Sorry array does not work either haha
I said a wrong things

@Thiagolino8
Copy link

@ryoppippi

When we use class, it works (looks like singleton by @7nik )

Not really, his version is a singleton because it always returns the same instance, your version always creates a new instance
And even his solution is not very practical

@ryoppippi
Copy link

ryoppippi commented Nov 16, 2023

@Thiagolino8

Not really, his version is a singleton because it always returns the same instance, your version always creates a new instance
And even his solution is not very practical

Oh yeah so we cannot share the instances btw components when we just import the other.svelte.js scripts
So we need to pass the class instance by Components' props for now.

like this

@peterreeves
Copy link
Author

This is fixed now as a result of #9739 being merged! 🎉

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

No branches or pull requests

8 participants