fix: destroy each items after siblings are resumed#17258
Conversation
🦋 Changeset detectedLatest commit: a64d391 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Marked as draft, because I'd like to fix the remaining issues as well if I can |
|
Alright, this turned into a far more comprehensive refactor. But we finally have an each block implementation that appears to pass the stress test: The most obvious change in the diff is the |
|
Gah, spoke too soon. Found a case that fails the stress test. We're close, I can feel it — this branch gets way further into the test than anything else we've tried. Will come back to this tomorrow. |
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
I admit I don't fully understand all of this... but from my current level of understanding it looks good. I don't love having "yo please run this test if you feel like it" as part of our workflow though
| // When making substantive changes to this file, validate them with the each block stress test: | ||
| // https://svelte.dev/playground/1972b2cf46564476ad8c8c6405b23b7b | ||
| // This test also exists in this repo, as `packages/svelte/tests/manual/each-stress-test` |
There was a problem hiding this comment.
I don't love this... is there a straightforward way we could automate stress testing when this file is changed?
There was a problem hiding this comment.
I'm sure there's a way, but I'm not so sure about 'straightforward'. Even converting it to a test that could be part of our test suite would be difficult, I think
|
I suspect this is just cross-talk (ie pressing the button causes an update that the ongoing test doesn't expect) |
|
Just came here to say thank you for this update!! Spent about a week trying to fix a bug that had to do with this, when I didn't know it was due to this issue in Svelte. Updated packages to latest and the issue was completely gone with my lists. So thank you!!! ❤️ That is all. Ya'll don't get thanked enough ❤️ |


This fixes ones of the bugs identified in #17240 (comment). (To be honest I'm astonished that it lasted this long.) Specifically, when you have an
eachblock go from['a', 'b']to[], and then to['a']while items are still outroing,bwill never be removed from the DOM, because theaoutro is aborted and the callback that destroys effects is never called.This solution is admittedly a bit more involved than I initially hoped. I wonder if it would be better to coordinate effect destruction at the batch level, though I didn't pursue that as it would be a breaking change. At least this change imposes no costs on
eachblocks that don't contain outros.Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint