fix: overhaul each blocks
#17150
fix: overhaul each blocks
#17150
Conversation
🦋 Changeset detectedLatest commit: e41a8ea 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 |
|
|
Need to come up with some test cases that test tricky things like out-of-order updates, but this is already way more robust than (there is still a bug insofar as it begins at |
…nding (#17151) Co-authored-by: dylan <dylanbradshaw107@hotmail.com> Co-authored-by: Rich Harris <rich.harris@vercel.com>
|
Thanks — have merged the test in that PR into this branch. There's more that can be done here as we discover test cases, but since it fixes some stuff already (and apparently doesn't regress anywhere) I've marked it ready for review. |
dummdidumm
left a comment
dummdidumm
left a comment
There was a problem hiding this comment.
This is looking great! I've added another test. The pending count is off as you say (in the test, too), I think it's because of the array entries having multiple variants in later batches - e.g. when you push 2 then 3 the first batch will see [1,2] and the second one [1, undefined, 3], and so the repeated entry will create another pending entry. It doesn't quite line up for later iterations though. I don't know how to best fix it yet, one possibility might be to special-case each blocks and require each change to happen on their own, or somehow merge later iterations into earlier ones.
This also doesn't fix #17050 (hoped it did since it's also about each blocks, but repro still fails), and there's some pending TODOs (especially the one about discard doing nothing concerns me with regards to forking) but that doesn't need to block this PR IMO.
|
cool — will go ahead and cut a release with this, then open a PR that unskips that new test. The follow-up work can start there |
|
|
||
| if (item === undefined) { | ||
| var pending = offscreen_items.get(key); | ||
| item = /** @type {EachItem} */ (items.get(key)); |
There was a problem hiding this comment.
I am running into a bug in my codebase due to this line making the assumption that item cannot be undefined.
I am not able to make a reproduction, should i open a bug report anyway ?
There was a problem hiding this comment.
Yes please, maybe someone else is able to make one, or you find a way, a non-minimal reproduction would also be fine in that case
There was a problem hiding this comment.
I did find a reproduction #17166 (as you said). Thx for your help
As noted in #17126,
eachblocks are a little buggy when there's async stuff involved. #16977 fixed a bunch of stuff around async blocks/branches, but excludedeachblocks because, well, they're complicated. But the time has come to deal with it.As the commit messages suggest this is very WIP — a bunch of tests are failing, and while it fixes part of the problem (item effects and the fallback effect both need to be created immediately inside the
blockeffect, not later uponcommit), it doesn't yet fix the problems relating to overlapping async batches. But I thought I'd open the PR in this unfinished state anyway. Pleasingly, so far it's a net reduction in code and complexity.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