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

Action called a second time when the order of items changes in a keyed each #2735

Closed
matthieugicquel opened this issue May 10, 2019 · 6 comments
Labels

Comments

@matthieugicquel
Copy link

REPL link

<script>
  let items = ["a","b","c"];
  const on_click = () => items = ["a","c","b"];
	
  const action = (node) => {
    console.log("action init", node);
    return { destroy: () => console.log("action destroy", node) }
  };
</script>

<button on:click={on_click}>click</button>
{#each items as item (item)}
  <h2 use:action>{item}</h2>
{/each}

In this example, after I click the button, action() is called a second time for node c, even though this is the same DOM element that has just been moved around/mounted again. destroy() is not called.

@matthieugicquel
Copy link
Author

matthieugicquel commented May 10, 2019

Here's what I found found about how this happens:

In the update_keyed_each() code, the block for c is rightly identified as a will_move block, and this causes that code to run:

function insert(block) { 
  /***/
  block.m(node, next);
} 

/***/
else if (!lookup.has(new_key) || will_move.has(new_key)) {
  insert(new_block);
}

In the compiled code of the component, I can see that the action calls are in the m function:

m(target, anchor) {
/***/
action_action = ctx.action.call(null, h2) || {};
},

I'm not sure about the way this should be fixed. Some questions:

  • Are there others cases (aside from keyed each blocks) where svelte "mounts" components that are, in fact, already mounted?

  • What's the right choice here?

  1. Call destroy(), and then call action() again
  2. Make sure action() is called only once?

@mrkishi
Copy link
Member

mrkishi commented May 11, 2019

Oh... This is tangentially related to #2446. Bindings, actions and event listeners currently do not have a set order. One way to fix this would require moving actions to create and applying them all in order. This issue would be fixed by that.

The issue is that this would be a breaking change for actions since the dom tree would not be complete yet. Actions would be called on creation, so they'd need a way to be alerted of mounts (return { mount, update, destroy }).

The backward-compatible solution would be to move bindings and event listeners to mount and applying them all in order. We'd then we run into this issue for them as well, so we would indeed need to only do these once.

@mrkishi
Copy link
Member

mrkishi commented May 18, 2019

This also seems to be related to #2711 — the issue seems to boil down to mount not being idempotent but being called multiple times.

@jacwright
Copy link
Contributor

I am also experiencing this issue and created a simple repro case if that helps to fix it.
https://svelte.dev/repl/b796b0c7e6ff4892adebfecbf189fc80?version=3.12.1

@tanhauhau tanhauhau added the each label Mar 16, 2020
@tanhauhau
Copy link
Member

Related #4491

@matthieugicquel
Copy link
Author

Closing this, fixed by #4493 and included in v3.20.0

Thanks @tanhauhau!

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

5 participants