sveltejs / svelte

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

Binding to an object fires erroneous reactive update #5689

Closed intelcentre closed 5 months ago

intelcentre commented 3 years ago

Describe the bug When binding a complex value such as an object to a component <Component bind:value />, additional reactive updates are fired for the bound value even when no data is changed.

May be related to https://github.com/sveltejs/svelte/issues/4430 ?

To Reproduce Simply bind a complex value to a component property.

Example: https://svelte.dev/repl/5e14759de70d4d39b6f3833f91db4542?version=3.29.7

Expected behavior Reactive updates should be consistent between simple and complex types

Severity I find it irritating, but not a blocker. This issue is most likely to cause redundant calculations without formally breaking anything. This issue will likely snowball if components are nested, though I have not confirmed this. The additional updates are likely to confuse people causing unnecessary work.

Additional context Add any other context about the problem here.

pushkine commented 3 years ago

likely duplicate #4447

xpuu commented 3 years ago

similar problem reported here #5555, but this is much cleaner example.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gyurielf commented 2 years ago

it is still actual!

SarcevicAntonio commented 2 years ago

I ran into this as well.

Conditionally rendering the binding will refire the reactive statement.

Here is a reproduction: https://svelte.dev/repl/ed4f4a3d1d694e92a03cd08a3b5ce0a0?version=3.48.0

gyurielf commented 2 years ago

I went around the issue and found Kevin Bridges awesome explanation about the entire reactivity. So every non primitives are counts as "dirty". That's why we ran into this.

kevinleto-informaticon commented 2 years ago

I run into this as well. Thanks @gyurielf for the link to Kevin's explanation, now it's clear why that happens.

What isn't clear to me yet, is if that is desired... Should Svelte work like that with objects or is it a bug? I don't see any use case that requires the object to be invalidated entirely as soon as one single property is updated. I wouldn't expect that.

For example: I want reload something when the user name is updated

<script lang="ts">
  export let user: {name: string, age: number};// ← comes from the main component through "bind:user"

  $: nameChanged(user.name);
  function nameChanged(newName) {
    reload(); // ← my logic
  }

</script>

The code needs the following changes to work as expected:

<script lang="ts">
  export let user: {name: string, age: number};

  let oldName: string; // ← variable to store the old value

  $: nameChanged(user.name);
  function nameChanged(newName) {
    if(oldName == undefined || oldName !== newName) { // ← check value changed
      oldName = newName; // ← update the old value
      reload(); // ← my logic
    }
  }

</script>

I think that is a strange workaround to make it all work.

PatrickG commented 2 years ago
<script lang="ts">
  export let user: {name: string, age: number};// ← comes from the main component through "bind:user"

  $: nameChanged(user.name);
  function nameChanged(newName) {
    reload(); // ← my logic
  }

</script>

This will never work, even without this bug. nameChanged will be called at least once on initialization.

kevinleto-informaticon commented 2 years ago
<script lang="ts">
  export let user: {name: string, age: number};// ← comes from the main component through "bind:user"

  $: nameChanged(user.name);
  function nameChanged(newName) {
    reload(); // ← my logic
  }

</script>

This will never work, even without this bug. nameChanged will be called at least once on initialization.

I know. I've simplified the code to make it as simple as possible. A simple if(user) { ... } makes the trick. The focus was on the workaround 😄

SarcevicAntonio commented 2 years ago

Should Svelte work like that with objects or is it a bug?

IMO, in an ideal world, Svelte would be as smart and lazy as possible, only doing the work needed if something (even object properties) actually changed.

I'm sure there is a good reason it's this way right now, but maybe that could change in the future?

gyurielf commented 2 years ago

What isn't clear to me yet, is if that is desired... Should Svelte work like that with objects or is it a bug? I don't see any use case that requires the object to be invalidated entirely as soon as one single property is updated. I wouldn't expect that.

IMHO a reasonable decision for the deeply nested objects. It's too expensive to get around all of the deeply nested props. So I guess that's why.

infuzz commented 2 years ago

I am facing exactly the same unexpected behavior. I made a a very simple REPL to show the issue with the minimal setup.

Does anybody found a workaround to this ? (In my produsction case, this behavior leads to a double fetch...)

kevinleto-informaticon commented 2 years ago

You probably need to work with boolean variables like:

<script>
  let alreadyLoaded = false;
  if(!alreadyLoaded) {
    data = fetch(...);
    alreadyLoaded = true;
  }
</script>

That is the only workaround I've found till now.

Prinzhorn commented 2 years ago

I went around the issue and found Kevin Bridges awesome explanation about the entire reactivity. So every non primitives are counts as "dirty". That's why we ran into this.

Svelte's reactivity is based on assignments. This issue is about the object being marked dirty for no reason, without any assignment. So I don't think this is relevant.

What isn't clear to me yet, is if that is desired... Should Svelte work like that with objects or is it a bug?

What you are suggesting is that changing an object property should not invalidate the entire object and that's a fair point to make when first encountering this behavior. But think about this: the object might have getters or methods that depend on multiple properties, it's impossible for Svelte to know. If you know, then look into the immutable option. Also what about obj[key]? Svelte cannot know what keys you use at runtime. By default the correct way is to invalidate the entire object. However, this is not related to this issue at all. This issue is about the object being invalidated for no reason at all.

I don't see any use case that requires the object to be invalidated entirely as soon as one single property is updated.

In addition to what I said above, there are countless of use-cases. Such as sending the object to a server, syncing it via BroadcastChannel or caching it in sessionStorage. All if which I do (abstracted via stores). For me this behavior is desired and absolutely vital. Usually you don't have to worry about it, because all the places the other properties of the object are used and passed around will eventually compare with a primitive and result in a NOOP (that's partially what "surgically updates the DOM" is about). But again, not related to this issue. Any discussion about it would probably be better of in a different place to not water down this bug.


Again, here's a minimal REPL that this issue is about: https://svelte.dev/repl/5f436e05809346a38baf906184321761?version=3.49.0 This will log obj twice. For absolutely no reason. In complex applications this causes a lot of undesired and expensive side-effects such as #6298

I re-iterate:

  1. Svelte's reactivity is based on assignment
  2. There is no assignment happening at all, the obj is not touched whatsoever
  3. Simply using bind with an object will cause the object to be invalidated for no reason
  4. This breaks assumptions about Svelte and is a bug causing performance issues and side-effects in real applications
trungtin commented 2 years ago

There is another related problem that I have. Reproduce here https://svelte.dev/repl/c6c7a555d8e24572ae00461bb989fee9?version=3.49.0

Basically I want:

  1. Init form state from some input data
  2. Re-computed the form state if the input data change
  3. Bind the form state to the element value

If there is a line of code that reassign the input data (line 6 in the repl), even if that line isn't executed, the form state still gets re-computed every time. Commented out that line, then it's work.

probablykasper commented 1 year ago

In my use case, this is actually a pretty inconvenient problem. I have a big complex object that I subscribe to, and when it's updated I show an "Unsaved changes" indicator. Because of this issue, the "Unsaved changes" indicator can show up simply by mounting a component.

baseballyama commented 1 year ago

Fixed by https://github.com/sveltejs/svelte/pull/7981

Conduitry commented 1 year ago

This should be fixed in 3.54.0 - https://svelte.dev/repl/5e14759de70d4d39b6f3833f91db4542?version=3.54.0

baseballyama commented 1 year ago

I reopen this issue and I removed the "bug" label. I described the reason at https://github.com/sveltejs/svelte/issues/6298#issuecomment-1368674996.

Prinzhorn commented 1 year ago

I'm confused. Why re-open it then? Either it's a bug or not. Can you explain the purpose of the "conservative reactivity" label? Is this relevant for v4?

A reactive statement is always executed whenever there is a possibility of a change in the object's properties.

There is no possibility, because there is no assignment. I pass a prop down and nothing should come back up.

If I understand #8114 correctly the changes were reverted because you couldn't find a solution (that doesn't break other things) and not because you concluded it's not a bug.

I was luckily able to workaround #6298 by using clone + deep-equal, but that's not something that would work without stores. And you also first need to realize that something is going bad. In my case simply rendering a specific component would trigger dozens (depending on the number of items in the store) of network requests for no reason at all. And every change of a single item would again trigger a request for all items because the binding keeps invalidating everything for no reason.

baseballyama commented 1 year ago

Is it an answer to your question? https://github.com/sveltejs/svelte/pull/8114#issuecomment-1368788459

Can you explain the purpose of the "conservative reactivity" label? Is this relevant for v4?

Of cause, such unnecessary processes should not be executed. But this is an intentional trade-off between performance and this issue. For example, if we use a Proxy, we can prevent executing such a process, but we need to pay for Proxy overhead. Our current consensus is that unnecessary event firing is better than such overhead because reactive things should have idempotence and we have some workaround for this.

"conservative reactivity" means the downside of this trade-off. In Svelte4, I'm not sure that we will improve this point.

aniolekx commented 1 year ago

But this is an intentional trade-off between performance and this issue.

this is not a trade-off ====> this is insane... behaviour of component should be predictive, not random, I was not aware of this issue, and wasted many hours trying to understand why I have these extra extra components updates

I am using Svelte with Symfony + Inertia, and when props are passed to top level component, I already have double rendering, just by passing non primitive prop like object or array.

jaydeep987 commented 1 year ago

This is so shame to know about this issue. and way we bind prop and also assign value to it as reactive prop is widely useful. but bcoz of this issue it can easily produce tons of bugs and performance issues. And in this many years, it still not solved. Hardly we convinced to use svelte, and such basic stuff issue causes lot of negativity in team. bcoz we need to use dirty workarounds like _oldblahblah and checks.

jsilveira commented 1 year ago

I would love a github reaction with the semantic "I also wasted a lot of time because of this unexpected behaviour, give it one more dev higher priority when rethinking reactivity". Meanwhile, I'll just leave this comment. Thanks

frederikhors commented 1 year ago

@Rich-Harris, we just had another bad discovery in a project that we got from others and that we can't fix rapidly (we can't think of any work-arounds to this problem other than rewriting everything differently).

As @Prinzhorn well explained we are all a bit desperate at this point because we love Svelte a lot but when you start creating more complex projects you come across this issue and other issues that have been stuck for years now!

PLEASE, ONE QUESTION ONLY: Could you tell us where in the future roadmap is the resolution of this issue?

I'm not asking for an ETA, just where in the future program it is.

I'm trying to keep the client and my team from switching to Vue! :pray:


@Prinzhorn did you found an alternative way?

gauel commented 1 year ago

I was wondering why is my reactive function firing so many times and just discovered this... :-(

dummdidumm commented 10 months ago

This will be fixed in Svelte 5, only one update happening now. Related to #4265