sveltejs / svelte

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

onDestroy recreates binded property item in array that has been spliced #9314

Open ferulisses opened 11 months ago

ferulisses commented 11 months ago

Describe the bug

When using array.slice to remove a item from a list, the item is recreated by onDestroy on sub component that have the property binded to it.

Reproduction

I created a small project to reproduce the error, click on any button to remove an item, the result is a looping: https://svelte.dev/repl/bcc1d85ebf4c4e04b98971536e0c4454?version=4.2.1

Logs

No response

System Info

Svelte 3.59.2, Chrome 117

Severity

annoyance

esbeto commented 11 months ago

Just change bind:people to {people}. Or maybe update the Issue title and description, since the issue arises only if you're modifying a binded property onDestroy that is getting spliced.

ferulisses commented 11 months ago

Hi, Just tested the proposed change, same result: https://svelte.dev/repl/c189220485144865b04b8e99e5fbdf96?version=4.2.1

esbeto commented 11 months ago

I mean this: https://svelte.dev/repl/7d91c4ba040747d39ee7c09a4f00e973?version=4.2.1

ferulisses commented 11 months ago

Hum, but if it's no binded I can't change the property content inside the sub component.

Don't let the simplicity of the example on the way, the real application is more complex than that.

I thought in a workaround that probably will fix it for me: https://svelte.dev/repl/4c91070177114932b0a72c9964d17c89?version=4.2.1

My question now: is this a bug or should be implemented in another way? If it's a bug, can it be fixed?

esbeto commented 10 months ago

I mean, there's something really weird about changing a property of an object that just got spliced, no? It's like trying to access something that is not there anymore.

This is what you're doing in my mind (let's simplify with todos instead):

Let's say you have some todos

let todos = [
  { done: false, text: 'finish Svelte tutorial' },
  { done: false, text: 'build an app' },
  { done: false, text: 'world domination' }
];

Then you delete the second one:

todos.splice(1, 1);

Then you expect Svelte to modify the deleted todo?

onDestroy(() => {
  todo.done = true;
})

What's gonna happen next?

let todos = [
  { done: false, text: 'finish Svelte tutorial' },
  // { done: true, text: '👻 ghost todo that got deleted' },
  { done: false, text: 'world domination' }
];

Maybe you have to rethink a bit what you're trying to do.

esbeto commented 10 months ago

If you're trying to modify the deleted todo, (because of some reason, let's say logging). Then you should do it before splicing.

You have your todos:

let todos = [
  { done: false, text: 'finish Svelte tutorial' },
  { done: false, text: 'build an app' },
  { done: false, text: 'world domination' }
];

Then, on delete you do the work you expect to do (without relying on onDestroy)

function deleteTodo(index) {
    todos[index].done = true;
    // Perform some work
    console.log(todos);
    // Splice, reassign or just filter....
    todos = todos.filter((todo, idx) => idx !== index);
    // Perform more work
    console.log(todos);
}
ferulisses commented 10 months ago

You are right that it's weird to try to change something that has been deleted.

What is odd to me is when I delete an item from an array, it's recreated by a method that try to modify something that has been deleted, and create some infinite looping like in the first example that I posted (remove any item except the last one): https://svelte.dev/repl/bcc1d85ebf4c4e04b98971536e0c4454?version=4.2.1

I would expect an Exception to be generated.

esbeto commented 10 months ago

I think the reason you're not getting an error and getting an infinite loop is because of the odd combination of things that are happening. You splice the array. But before this happens the animation must play. So it's very likely that Svelte keeps the item before splicing it for animation and defers the splicing after the animation, but then you have an onDestroy method that modifies it, so it gets confused, it tries to splice it, but it's modifying it, but the animation wants to play, but its actually being deleted, and so on and so forth.

By the way, if I remove the animation I do get an error: Screenshot 2023-11-22 at 15 35 34