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

Svelte 5: store updates trigger component prop changes that didn't with 4 #11448

Closed
CaptainCodeman opened this issue May 3, 2024 · 12 comments · Fixed by #11577
Closed

Svelte 5: store updates trigger component prop changes that didn't with 4 #11448

CaptainCodeman opened this issue May 3, 2024 · 12 comments · Fixed by #11577
Assignees
Milestone

Comments

@CaptainCodeman
Copy link

CaptainCodeman commented May 3, 2024

Describe the bug

Setting a component prop to a store value causes the component prop to see a change with 5 when it didn't with 4.

i.e. it appeared to be deduped with 4 but isn't with 5

Reproduction

Svelte 4: https://svelte.dev/repl/d43370affcf9403b9a5566a9e9849da8?version=4.2.15

Toggle triggers a change in the component, Update doesn't (prop value stays the same).

Svelte 5: https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE41SQW6DMBD8ytaKFJAQ3BEgVflCeyo9ELJEVo3XwkuayvLfK-yIpEkPOa09npkd7dqJQSq0ovxwQncjilK8GiMywT9mudgTKkaRCUvz1C9IZftJGm5a3bIcDU0MDr4nyd1eIXgYJhphG3WFZZpwe0Pd0WhIo-YLLy9WJI-aha1b7klbhqCHevVPHJw6NWMJQ6csgk8je5h1z5I0MB2PCpMU3AK3HAzy2Rw6xuQMdQNXi5dzHk7g08WG_Z3XRfWU16NVVVwHpav9zEwaSJe9kv1X7WJQ37yFWhWR0PxHjR198x7qDbXVbhNDheYhf3WdcADrvxQoGpGJkQ5ykHgQJU8z-mzd_Sp-9gfgOaxV4aVdHOGmhGV_pDBXdEzCS3o3EhcDPab59L-vjIN4lQIAAA==

Update now triggers a change of the prop value, even though the value hasn't actually changed (see console log)

Logs

.

System Info

System:
    OS: macOS 14.4.1
    CPU: (6) x64 Intel(R) Core(TM) i5-8500B CPU @ 3.00GHz
    Memory: 1.83 GB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.0.0 - ~/Library/pnpm/node
    npm: 10.5.1 - ~/Library/pnpm/npm
    pnpm: 9.0.6 - ~/Library/pnpm/pnpm
    bun: 1.0.0 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 123.1.64.113
    Chrome: 124.0.6367.119
    Chrome Canary: 126.0.6456.0
    Safari: 17.4.1
    Safari Technology Preview: 17.4
  npmPackages:
    svelte: ^5.0.0-next.123 => 5.0.0-next.123

Severity

blocking an upgrade

@paoloricciuti
Copy link
Contributor

paoloricciuti commented May 4, 2024

It's actually more broad than that...is just that by changing the reference to the base object you are actually invalidating the whole effect. The same happen with simple reassignment

https://svelte.dev/repl/e9ba4f7ea0e74285a3b66ebfd98a7cae?version=4.2.15

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE1WOQQrCMBBFrzIMLlIo6rq2Bc9hXNQ4lWCclGYiSMjdpVFRl_OZ9_5POFpHAZtDQh5uhA3upwlrlMe0HOFOTghrDD7OZknaYGY7Sa9ZiyMB4yMLdLAKMgipBPfBRWpgC7naaV7eVjSOZESpCroe0hJpMZ6Dd7R2_qKKY13AwkhB2823ittTFPEMno2z5tolVXV9UX0GpHfxjyxrzrksTf9pu3npeqzx5s92tHTGRuZI-ZifGKSDjBQBAAA=

However i wonder if this should just be considered a breaking change, personally i don't have a good idea of how this could work with signals. I'll thinker a bit about it while we wait for maintainers to show up.

Scratch that i'm an idiot and svelte 4 actually has the same behavior in this case.

@CaptainCodeman
Copy link
Author

CaptainCodeman commented May 4, 2024

Yes, any update to the store, even another property on an object, can end up triggering a prop change in a component when its values haven't really changed.

IMO it's a breaking change because it can easily alter the behavior of existing components when running under 5 vs with 4, even when runes are not used.

Although it's fairly easy to dedupe the changes within a component, those components might be 3rd party packages.

Update: I think my comment overlapped your edit - we're saying the same thing.

@CaptainCodeman
Copy link
Author

Svelte 5 has the same behavior in runes mode - i.e. the prop updates. So if that is an intended change it should be noted as breaking #11400

But for non-runes mode, it needs to behave as 4 did IMO.

@dummdidumm dummdidumm self-assigned this May 6, 2024
@dummdidumm
Copy link
Member

Is this something that causes real bugs in practise? I'm contemplating whether or not we should fix this, because it will likely mean loads of extra deriveds (one for each prop) in legacy mode for what I believe is a marginal fix in edge cases.

@dummdidumm dummdidumm added this to the 5.0 milestone May 6, 2024
@gterras
Copy link

gterras commented May 6, 2024

Is this something that causes real bugs in practise? I'm contemplating whether or not we should fix this, because it will likely mean loads of extra deriveds (one for each prop) in legacy mode for what I believe is a marginal fix in edge cases.

It seems like a situation that would crucially rely on this effect should also have some kind of lifecycle or reactive check to work properly in most cases anyway.

@CaptainCodeman
Copy link
Author

Yes, I found it from my first attempt upgrading some apps to 5. It isn't super-difficult to handle the change of behavior within the components but there is a real possibility that some components will break because of it and they might be external components.

@CaptainCodeman
Copy link
Author

It seems like a situation that would crucially rely on this effect should also have some kind of lifecycle or reactive check to work properly in most cases anyway.

Not sure what you mean - something relying on the existing v4 behavior?

IMO it's consistent in v4 - setting a component prop to the value it is already set to doesn't trigger a change whether that prop value comes from a reactive variable or a store. I think it would be confusing if components changed their behavior based on how the property value was passed in from being a reactive variable, store, or rune.

@dummdidumm
Copy link
Member

Yes, I found it from my first attempt upgrading some apps to 5. It isn't super-difficult to handle the change of behavior within the components but there is a real possibility that some components will break because of it and they might be external components.

Could you elaborate what exactly broke after upgrading, or put differently, how your real component looked like so that it broke?

@CaptainCodeman
Copy link
Author

CaptainCodeman commented May 7, 2024

The specific component applied some CSS animations in a sequence when it saw a boolean toggle change (when it hadn't actually changed, but something else in the same store managing the state had).

I haven't tested it with a string value yet but I expect it will affect a deep-zoom photo viewer that relies on the change to a base URL to trigger a reset of the view and cause it to start loading image tiles (where a store could update for other reasons, like registering a 'like').

I need to also test a lazy image component that shows a blur-hash image while the proper image loads as again, the URL changing is the trigger to start the process and the store that has it can be updated for other reasons (e.g. changing sizing / positioning)

It's really anything where the prop-change is a trigger to something happening which is more than simply rendering values into the component template. I don't tend to use it, but could see people fetching data in response to data changing.

Maybe it needs a "strict" vs "simple" configuration setting for legacy mode, where people could start with the strict mode when converting to reduce the number of issues, and switch to the altered behavior as a separate optional stepping stone to moving to native runes mode?

@gterras
Copy link

gterras commented May 7, 2024

The specific component applied some CSS animations in a sequence when it saw a boolean toggle change (when it hadn't actually changed, but something else in the same store managing the state had).

I haven't tested it with a string value yet but I expect it will affect a deep-zoom photo viewer that relies on the change to a base URL to trigger a reset of the view and cause it to start loading image tiles (where a store could update for other reasons, like registering a 'like').

I initially thought you were passing the store (and meant what you did with createDeduper since this kind of tasks usually already involve storing side data) but reading the example again it's clear that it should be listed as a breaking change especially since it's less intuitive, as in not necessarily creating bugs (although I'm curious to see the animation case) but could easily trigger unnecessary workload in many reasonable implementations.

@CaptainCodeman
Copy link
Author

CaptainCodeman commented May 7, 2024

No, it's passing a property value from the store when some other update is made - the deduper was to prevent the update triggering a prop change where there wasn't really any change to the actual prop value itself. The complication is when the component may happen to be deeper within some 3rd party package you import and don't control or you just don't understand why your app is behaving strangely.

The animation is playing enter / leave transitions via switching TailwindCSS classes, based on a toggle property and used in UI components which would suddenly start flickering and behaving weirdly as other properties were set in the stores containing the state. i.e. changing the selected item on a menu would trigger the menu to re-run the open animation when it shouldn't. I fixed it in the component but it could be a gotcha when anyone is upgrading from 4 to 5.

I'm part way through dealing with some separate build issues, but think there are a few other places in my apps that could be impacted by this. Some may not be visible, but the updates could trigger things like Firestore subscriptions to re-fire, which is generally something to avoid ($)

dummdidumm added a commit that referenced this issue May 13, 2024
dummdidumm added a commit that referenced this issue May 13, 2024
Rich-Harris added a commit that referenced this issue May 13, 2024
* fix: replicate Svelte 4 props update detection in legacy mode

fixes #11448 by wrapping props in deriveds

* fix test

* Update packages/svelte/src/compiler/phases/3-transform/client/utils.js

Co-authored-by: Rich Harris <[email protected]>

* dedicated flag

* prettier

---------

Co-authored-by: Rich Harris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment