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

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

Closed
RyanK1m opened this issue Jan 15, 2020 · 41 comments
Closed

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

RyanK1m opened this issue Jan 15, 2020 · 41 comments

Comments

@RyanK1m
Copy link

RyanK1m commented Jan 15, 2020

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:

  • Your browser and the version: Chrome Version 79.0.3945.117 (Official Build) (64-bit)

  • Your operating system: OS X 10.14.6

  • Svelte version 3.17.1

  • Rollup

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

@RyanK1m RyanK1m changed the title Double bind causes reactivity update twice when update value inside of an object or an array. Double bind causes reactivity update twice when bind array or object. Jan 15, 2020
@RyanK1m RyanK1m changed the title Double bind causes reactivity update twice when bind array or object. Reactivity update twice when bind array or object on a component. Jan 15, 2020
@soullivaneuh
Copy link

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

damooo commented Feb 24, 2020

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

@silllli
Copy link

silllli commented May 15, 2020

I encountered the same issue.

@ruslan-khomiak
Copy link

I have the same problem

@etnilo-indra
Copy link

I have the same problem at v3.23.0

@pwwang
Copy link

pwwang commented Aug 13, 2020

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

@Conduitry Are you going to fix this?

@antony
Copy link
Member

antony commented Oct 20, 2020

@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

@antony antony closed this as completed Oct 20, 2020
@pwwang
Copy link

pwwang commented Oct 20, 2020

@superseby2
Copy link

superseby2 commented Oct 24, 2020

@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
Copy link
Member

antony commented Oct 24, 2020

@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
Copy link

sourcecaster commented Nov 21, 2020

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
Copy link
Contributor

zqianem commented Nov 22, 2020

@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

zqianem added a commit to byrd-polar/fluid-earth that referenced this issue Nov 22, 2020
Workaround for the behavior in the second REPL here:

sveltejs/svelte#4265 (comment)

causing two gridded data fetches to happen per change instead of one.
(And fast enough that the Fetcher object doesn't seem to be able to
abort them? Might be a separate issue.)

Also a good time to learn how to use stores, I guess. I think having the
bound variables is clearer in this case so we may want to revert this
once the Svelte bug is fixed, but stores are still pretty neat.
@Fry98
Copy link

Fry98 commented Nov 25, 2020

@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
Copy link

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

xpuu commented Dec 1, 2020

@MadSheogorath I think it'd be duplicate of this #4447

@yjp20
Copy link

yjp20 commented Feb 24, 2021

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

@AiziChen
Copy link

AiziChen commented Aug 9, 2021

@sourcecaster
Copy link

sourcecaster commented Aug 10, 2021

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
Copy link
Contributor

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

@antony antony reopened this Dec 11, 2021
@xpuu
Copy link

xpuu commented Dec 12, 2021

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
    })
  : []

@AgarwalPragy
Copy link

3.55.1 and counting!

@aniolekx
Copy link

aniolekx commented Mar 1, 2023

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

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

@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
Copy link

alimo commented Mar 23, 2023

@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
Copy link

Xazzzi commented Jun 20, 2023

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

@hackape
Copy link
Contributor

hackape commented Jun 26, 2023

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
  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
    function update($$) {
    if ($$.fragment !== null) {
    $$.update($$.dirty);
    run_all($$.before_update);
    $$.fragment && $$.fragment.p($$.dirty, $$.ctx);
    $$.dirty = null;
    $$.after_update.forEach(add_render_callback);
    }
    }

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
Copy link
Contributor

hackape commented Jun 26, 2023

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.

export function flush() {
const seen_callbacks = new Set();
do {
// first, call beforeUpdate functions
// and update components
while (dirty_components.length) {
const component = dirty_components.shift();
set_current_component(component);
update(component.$$);
}
while (binding_callbacks.length) binding_callbacks.pop()();
// then, once components are updated, call
// afterUpdate functions. This may cause
// subsequent updates...
for (let i = 0; i < render_callbacks.length; i += 1) {
const callback = render_callbacks[i];
if (!seen_callbacks.has(callback)) {
callback();
// ...so guard against infinite loops
seen_callbacks.add(callback);
}
}
render_callbacks.length = 0;
} while (dirty_components.length);
while (flush_callbacks.length) {
flush_callbacks.pop()();
}
update_scheduled = false;
}
function update($$) {
if ($$.fragment !== null) {
$$.update($$.dirty);
run_all($$.before_update);
$$.fragment && $$.fragment.p($$.dirty, $$.ctx);
$$.dirty = null;
$$.after_update.forEach(add_render_callback);
}
}

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
    while (dirty_components.length) {
    const component = dirty_components.shift();
    set_current_component(component);
    update(component.$$);
    }
  5. while (binding_callbacks.length) binding_callbacks.pop()() calls child_values_binding and child_arr_binding
    while (binding_callbacks.length) binding_callbacks.pop()();
  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));
  }
  1. 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.
    while (dirty_components.length) {
    const component = dirty_components.shift();
    set_current_component(component);
    update(component.$$);
    }
  2. 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
Copy link
Contributor

hackape commented Jun 26, 2023

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
Copy link
Contributor

hackape commented Jun 26, 2023

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?

arnard76 added a commit to arnard76/rep-counter that referenced this issue Jul 3, 2023
…e multiple times

when variable data is an array (and objects too according to sveltejs/svelte#4265 (comment))

😲😲😲😭😭😭😭

the issue seems to be an issue with svelte for all svelte devs (not just me 😇).
Here is the issue page: sveltejs/svelte#4265
And here is a REPL for an idea of the issue: https://svelte.dev/repl/316c01d1b3b34a259b8fdba66f6ee14e?version=4.0.1

*The alternative* 😉🙌😄
replacing with update functions the update variable in parent component (similar to component binding) and event handlers (similar to element/input binding)

*other bits & bobs*😁😁
type safety to KeyRepArea.svelte component
arnard76 added a commit to arnard76/rep-counter that referenced this issue Jul 7, 2023
…e multiple times

when variable data is an array (and objects too according to sveltejs/svelte#4265 (comment))

😲😲😲😭😭😭😭

the issue seems to be an issue with svelte for all svelte devs (not just me 😇).
Here is the issue page: sveltejs/svelte#4265
And here is a REPL for an idea of the issue: https://svelte.dev/repl/316c01d1b3b34a259b8fdba66f6ee14e?version=4.0.1

*The alternative* 😉🙌😄
replacing with update functions the update variable in parent component (similar to component binding) and event handlers (similar to element/input binding)

*other bits & bobs*😁😁
type safety to KeyRepArea.svelte component
@Borisstoy
Copy link

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

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
Copy link
Member

antony commented Jul 15, 2023

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

7nik commented Jul 15, 2023

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

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
Copy link
Member

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

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

Successfully merging a pull request may close this issue.