-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix: destroy each items after siblings are resumed #17258
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |


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