sveltejs / svelte

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

Reactivity update twice when bind array or object on a component. #4265

Closed RyanK1m closed 1 month ago

RyanK1m commented 4 years ago

Describe the bug I'm trying to use double bind to retrieve values from a component. Bind simple value like integer, float, string works just fine. When the value inside of the component changes, a parent gets reactivity update from component only once. But if I want to bind an array or object, I got two updates from values changes inside of the component.

Logs NA

To Reproduce REPL: https://svelte.dev/repl/b0204a06a3f24ede8850017c8b20b998?version=3.17.1

  1. Press Test1 button
  2. updateCount1 should increase +1 but increases +2
  3. Press Test2 button
  4. updateCount1 should increase +1 and it increases +1

Expected behavior Double bind should cause reactivity update only once.

Stacktraces NA

Information about your Svelte project:

Severity This could potentially cause problems for users who want to track more complex objects from their components.

Additional context I'm trying to build a multi-value-selectable select component. https://svelte.dev/repl/4ddae394dcf841a687c76f6a9e702c8e?version=3.17.1 But I get reactivity update twice, and this can cause additional network usage if I want to hook value change(In this cause list of selected values) with an API

soullivaneuh commented 4 years ago

I ran onto this issue too, causing double fetch request at each change.

You see the implement here: https://gitlab.com/nexylan/svelty/-/merge_requests/9

Seems related to #3075 and #3197. Is it a bug or a miss-usage? In that case, there is any documentation about this?

Does anybody have a workaround for this issue?

damooo commented 4 years ago

@soullivaneuh i ran into same. v3.16.4 works as expected for temporary solution

silllli commented 4 years ago

I encountered the same issue.

ruslan-khomiak commented 4 years ago

I have the same problem

etnilo-indra commented 3 years ago

I have the same problem at v3.23.0

pwwang commented 3 years ago

This is seemingly solved at v3.24.1. But when that object/array is the argument of a function, the reactivity runs twice again:

https://svelte.dev/repl/5b37ba40c8854d86a9dab9ace0b6b861?version=3.24.1

ruslan-khomiak commented 3 years ago

@Conduitry Are you going to fix this?

antony commented 3 years ago

@Conduitry Are you going to fix this?

This works as expected in 3.29.0 - please try against the current released version before bumping issues.

https://svelte.dev/repl/e4ef6be683d84ff4a84d8c2c04703806?version=3.29.0

pwwang commented 3 years ago

This is still an issue:

https://svelte.dev/repl/5b37ba40c8854d86a9dab9ace0b6b861?version=3.29.0

superseby2 commented 3 years ago

@antony in your repl, make your testValue2 an object or make initial values as [] and you'll see 1 more count thanx expected.

Seeing the pb on 3.29.4 still.

antony commented 3 years ago

@superseby2 I've taken the original REPL and changed the svelte version and it works as expected. I then made your suggested changes and it still seems to work as expected.Can you provide a simple REPL which demonstrates the problem please so that we can see it?

@pwwang your REPL isn't a valid demonstration of the issues. You are getting two calls because you are initialising the variable prop with an empty array (call 1) and then binding it to a child component (call 2). If you correct your code to pass in the variable as you appear to have intended, it works fine. A REPL is only really useful if it doesn't contain any warnings or errors, and it is the simplest possible reproduction of the problem.

sourcecaster commented 3 years ago

I can confirm: 3.29.7 the problem is still there. If I set variable to simple value like: param = 100500 and then bind it - all works fine. Then change it to Array or Object - and it fires twice.

App.svelte

<script>
    import Component from './Component.svelte';
    let param = [100500];
    $: console.log(param);
</script>

<Component bind:param/>

Component.svelte

<script>
    export let param;
</script>

<span>just show param: {param}</span>

Console output

[100500] [100500]

zqianem commented 3 years ago

@MadSheogorath's example as a REPL: https://svelte.dev/repl/9b44ad8378f340cb87c1960a4b9b8caa?version=3.29.7

Some similar behavior that is possibly related: https://svelte.dev/repl/df6fe28c60e84c26a141debf305ed114?version=3.29.7 Opened a new issue for this

Fry98 commented 3 years ago

@pwwang your REPL isn't a valid demonstration of the issues. You are getting two calls because you are initialising the variable prop with an empty array (call 1) and then binding it to a child component (call 2). If you correct your code to pass in the variable as you appear to have intended, it works fine. A REPL is only really useful if it doesn't contain any warnings or errors, and it is the simplest possible reproduction of the problem.

@antony Maybe I just don't understand how reactivity in Svelte is supposed to work but that doesn't seem like an explanation to me. Why should binding trigger an update? The value didn't change, it just got bound to another component. It also doesn't get triggered when binding primitive data types.

sourcecaster commented 3 years ago

It's obvious from the latest example that behavior depends on variable type. Should we open a new bug? Looks like this one isn't tracked much since closed...

xpuu commented 3 years ago

@MadSheogorath I think it'd be duplicate of this https://github.com/sveltejs/svelte/issues/4447

yjp20 commented 3 years ago

It's not a duplicate; it seems related, but this example seems to show that it's still broken.

AiziChen commented 2 years ago

I solve it use onMount: https://svelte.dev/repl/cc81c52dcf034a83ac5f61cf68611037?version=3.6.4

sourcecaster commented 2 years ago

Looks like the bug isn't fixed still... https://svelte.dev/repl/9b44ad8378f340cb87c1960a4b9b8caa?version=3.42.1

I solve it use onMount

Nope, it is still there, just add $: console.log(prop); and you'll see it fires twice.

nbgoodall commented 2 years ago

@antony could you re-open this issue? It's still happening, with object bindings firing reactive statements twice (primitive values aren't affected).

Here's another REPL showing the difference between types in the console, on Svelte version 3.44.2.

xpuu commented 2 years ago

It seems that the problem might be here.

$$.ctx = instance
  ? instance(component, options.props || {}, (i, ret, ...rest) => {
      const value = rest.length ? rest[0] : ret
      // If value is not primitive not_equal is always true and component is marked dirty
      if ($$.ctx && not_equal($$.ctx[i], ($$.ctx[i] = value))) {
        if (!$$.skip_bound && $$.bound[i]) $$.bound[i](value)
        if (ready) make_dirty(component, i)
      }
      return ret
    })
  : []
alimo commented 2 years ago

Unfortunately, this issue still exists in 3.46.6. My workaround is to split the bind into a prop and an event to update the prop:

<Child
  value={complextObject}
  on:update:value={(e) => (complextObject = e.detail)}
/>

REPL

lominming commented 2 years ago

This is a super small example: https://svelte.dev/repl/37b572ef3fe44523a1bf1b8faddec66a?version=3.47.0

Bug still exists.

<script>
    import Child from "./Child.svelte"
    let arr = [123];
    let values = 123;
</script>

<Child bind:values bind:arr />
//Child.svelte

<script>
    export let arr;
    export let values;
    $: console.log(arr.length) //this will always run twice
    $: console.log(values) //this will run once
</script>
Mrbeyond commented 1 year ago

This bug still exists

KiddoV commented 1 year ago

Any update on this issue? I am still having this issue with version 3.49.0

AgarwalPragy commented 1 year ago

3.55.1 and counting!

aniolekx commented 1 year ago

https://svelte.dev/repl/37b572ef3fe44523a1bf1b8faddec66a?version=3.13.0 <--- works

seems that this behaviour was introduced in 3.14.0

https://github.com/sveltejs/svelte/blob/master/CHANGELOG.md#3140

this is really annoying because have lost many hours trying to understand when I have made mistake, and then this is just a bug, not related to my code.... I come form Vue2 world where where this seems to be more predicable

frederikhors commented 1 year ago

Unfortunately, this issue still exists in 3.46.6. My workaround is to split the bind into a prop and an event to update the prop:

<Child
  value={complextObject}
  on:update:value={(e) => (complextObject = e.detail)}
/>

REPL

@alimo do you mean by using https://svelte.dev/docs#template-syntax-component-directives-on-eventname and createEventDispatcher, right?

I don't recognize the syntax: on:update:value...

frederikhors commented 1 year ago

@Rich-Harris and others: I know you are working hard, guys!

I love Svelte with all my heart and use it where I can and tell everyone about it as soon as possible. BUT!

This small (BUT VERY BIG) problem breaks down all the biggest marketing actions you can do.

In some components (written without care) this problem causes us to repeat duplicate (and often quadrupled) queries with all the consequences of the case.

Don't abandon us on this.

Your dear followers, lovers of beautiful things.

alimo commented 1 year ago

@alimo do you mean by using https://svelte.dev/docs#template-syntax-component-directives-on-eventname and createEventDispatcher, right?

I don't recognize the syntax: on:update:value...

@frederikhors Yes, exactly.

Xazzzi commented 11 months ago

Wasted considerable amount of time tracking this down in 3.59.1. Painful experience.

hackape commented 11 months ago

Just want to note down my investigation findings so far on this issue.

Pointed out by aniolekx:

https://svelte.dev/repl/37b572ef3fe44523a1bf1b8faddec66a?version=3.13.0 <--- works

seems that this behaviour was introduced in 3.14.0

The change that introduces this behavior is this patch to file InlineComponent/index.ts

Using aniolekx's provided repl code as example,

<script>
    import Child from "./Child.svelte"
    let arr = [123];
    let values = 123;
</script>

<Child bind:values bind:arr />
<!-- 
    INSIDE Child...
    $: console.log(arr.length) //this will always run twice
    $: console.log(values) //this will run once
 -->

a comparison between compiled js by v3.13.0 vs v3.14.0 reveals below diff:

function create_fragment(ctx) {
  let updating_values;
  let updating_arr;
  let current;

  function child_values_binding(value) {
    ctx.child_values_binding.call(null, value);
-   updating_values = true;
-   add_flush_callback(() => (updating_values = false));
  }

  function child_arr_binding(value_1) {
    ctx.child_arr_binding.call(null, value_1);
-   updating_arr = true;
-   add_flush_callback(() => (updating_arr = false));
  }

  let child_props = {};

  if (ctx.values !== void 0) {
    child_props.values = ctx.values;
  }

  if (ctx.arr !== void 0) {
    child_props.arr = ctx.arr;
  }

  const child = new Child({ props: child_props });
  binding_callbacks.push(() => bind(child, "values", child_values_binding));
  binding_callbacks.push(() => bind(child, "arr", child_arr_binding));

  return {
    c() {
      create_component(child.$$.fragment);
    },
    m(target, anchor) {
      mount_component(child, target, anchor);
      current = true;
    },
    p(changed, ctx) {
      const child_changes = {};

      if (!updating_values && changed.values) {
+       updating_values = true;
        child_changes.values = ctx.values;
+       add_flush_callback(() => (updating_values = false));
      }

      if (!updating_arr && changed.arr) {
+       updating_arr = true;
        child_changes.arr = ctx.arr;
+       add_flush_callback(() => (updating_arr = false));
      }

      child.$set(child_changes);
    },

Below is the compiled js code of Child.svelte:

import { SvelteComponent, init, safe_not_equal } from "svelte/internal";

function instance($$self, $$props, $$invalidate) {
  let { arr } = $$props;
  let { values } = $$props;

  $$self.$set = ($$props) => {
    if ("arr" in $$props) $$invalidate("arr", (arr = $$props.arr));
    if ("values" in $$props) $$invalidate("values", (values = $$props.values));
  };

  $$self.$$.update = (changed = { arr: 1, values: 1 }) => {
    if (changed.arr) {
      $: console.log(arr.length);
    }

    if (changed.values) {
      $: console.log(values);
    }
  };

  return { arr, values };
}

class Child extends SvelteComponent {
  constructor(options) {
    super();
    init(this, options, instance, null, safe_not_equal, { arr: 0, values: 0 });
  }
}

export default Child;

All $: statements are compiled into code inside the $$self.$$.update(changed) wrapper function. $$.update is only called at two places:

  1. during the init phase of a component when it's first created https://github.com/sveltejs/svelte/blob/4c53eb6bda0d02f1f2cd3529791220eec4d8825c/src/runtime/internal/Component.ts#L139
  2. in the flush() internal phase which is an async batch updater that is scheduled to be called after any local state is modified and make_dirty'ed https://github.com/sveltejs/svelte/blob/4c53eb6bda0d02f1f2cd3529791220eec4d8825c/src/runtime/internal/scheduler.ts#L72-L81

There're plenty of dirty checks along the code path of flush to make sure it doesn't update thing that shouldn't be touched and only refresh those that are considered "dirty". if (changed.arr) inside Child's $$.update is one of those dirty checks.

init phase of Child component will call child.$$.update once, which is the one that we desire. But then the flush phase, which is triggered by binding to Child's exports from Parent component, will call child.$$.update again. And unfortunatedly the dirty check failed to guard this second call, thus we see this behavior that everyone complains about in current issue.

hackape commented 11 months ago

I'll focus on these three lines:

const child = new Child({ props: child_props });
binding_callbacks.push(() => bind(child, "values", child_values_binding));
binding_callbacks.push(() => bind(child, "arr", child_arr_binding));

read together with the source code of flush, we can understand what happens. https://github.com/sveltejs/svelte/blob/4c53eb6bda0d02f1f2cd3529791220eec4d8825c/src/runtime/internal/scheduler.ts#L34-L81

In chronological order, these things happen:

  1. new Parent({ target: ... }) leads to const child = new Child({ props: child_props });
  2. init of Child calls child.$$.update, which is desired
  3. init of Parent calls flush(), we enter the do ... while loop in flush
  4. 1st check dirty_components.length == 0, so this inner loop is skipped https://github.com/sveltejs/svelte/blob/4c53eb6bda0d02f1f2cd3529791220eec4d8825c/src/runtime/internal/scheduler.ts#L40-L44
  5. while (binding_callbacks.length) binding_callbacks.pop()() calls child_values_binding and child_arr_binding https://github.com/sveltejs/svelte/blob/4c53eb6bda0d02f1f2cd3529791220eec4d8825c/src/runtime/internal/scheduler.ts#L46
  6. child_arr_binding -> ctx.child_arr_binding.call(null, value_1); -> $$invalidate("arr", value_1); -> make_dirty(parent, "arr").
    function child_arr_binding(value_1) {
    ctx.child_arr_binding.call(null, value_1);
    -   updating_arr = true;
    -   add_flush_callback(() => (updating_arr = false));
    }
  7. Because step 6, by this point dirty_components[0] === parent, so we re-enter the do ... while. We're back to step 4, only this time inner loop is executed. https://github.com/sveltejs/svelte/blob/4c53eb6bda0d02f1f2cd3529791220eec4d8825c/src/runtime/internal/scheduler.ts#L40-L44
  8. Dive into update($$), it calls $$.fragment.p($$.dirty, $$.ctx) which translates to Parent's compiled code:

    p(changed, ctx) {
      const child_changes = {};
    
      if (!updating_values && changed.values) {
    +       updating_values = true;
        child_changes.values = ctx.values;
    +       add_flush_callback(() => (updating_values = false));
      }
    
      if (!updating_arr && changed.arr) {
    +       updating_arr = true;
        child_changes.arr = ctx.arr;
    +       add_flush_callback(() => (updating_arr = false));
      }
    
      child.$set(child_changes);
    },

This is where v3.14.0 diverges from v3.13.0!

In v3.13.0, updating_arr = true is set in child_arr_binding which is called at step 6, so at step 8, if (!updating_arr && changed.arr) dirty check will NOT pass, thus child_changes.arr = ctx.arr is NOT set. So changes will NOT propagate to child component via child.$set(child_changes) call, in turns child.$$.update() will NOT be called a second time.

In v3.14.0, updating_arr = true is NOT set in child_arr_binding, so at step 8 if (!updating_arr && changed.arr) dirty check WILL PASS, and erronously mark child_changes.arr = ctx.arr as dirty. So changes WILL propagate to child component via child.$set(child_changes) call, in turns child.$$.update() WILL be called a second time.

hackape commented 11 months ago

For those who just want a quick solution to the problem, add <svelte:options immutable={true} /> to the parent component should fix it.

Noted that this will also introduce unsatisfiying effect that forces you to create new object/array reference in order to notify the compiler about a change. See for defails: https://svelte.dev/docs/special-elements#svelte-options

hackape commented 11 months ago

I dig out the PR that introduce this behavior: #3886. It's meant to fix #3382, but unfortunately it fixed one problem and created another. šŸ«  I don't know what is the correct way to proceed either. One thing is for sure: two way binding is tricky.

@dummdidumm do you have time to do a triage on this issue?

Borisstoy commented 10 months ago

Adding my small contribution and testimonial here, facing this exact problem until finding this issue.

At least 6 hours banging my head against the wall !

Loving this framework, but this causes a lot of side effects to manage.

Mrbeyond commented 10 months ago

I don't expect this bug or issue to persist till now, after years of complaints.

The update (of binded variable that is of type object) is many as number of binds and nested binds.

That's a very bad.

Is it like the svelte team are not seeing this what?

antony commented 10 months ago

I don't expect this bug or issue to persist till now, after years of complaints. Is it like the svelte team are not seeing this what?

Please accept my apologies @Mrbeyond, I must have missed the PR you opened to fix this. Can you ping me a link when you have a minute?

7nik commented 10 months ago

Fixing this issue will most likely be a breaking change - I'm sure some cases depend on such behaviour.

Also, it makes sense to fix other reactivity-related issues altogether, but it's a really complicated thing.

I hope it will be done in Svelte 5.

Borisstoy commented 10 months ago

I would argue that this should considered as a hotfix and not wait for Svelte 5.

The current behavior should be preserved, and the fix should be introduced somehow as experimental.

dummdidumm commented 6 months ago

This will be fixed in Svelte 5, only one update happening now