fix: correct derived resolve order + don't process discarded batches#18177
fix: correct derived resolve order + don't process discarded batches#18177dummdidumm wants to merge 3 commits into
Conversation
Fixes part of sveltejs/kit#15431 This contains two fixes which combined make one of the tests pass. While doing so I realized that #18167 wasn't quite correct (because the changes that are correct and made the test pass regressed others) so I adjusted that, too. The first change is in `deriveds.js#async-derived`, where we don't just `break` when we come across our own batch, because that assumes that `deferreds` contains batches in incrementing order. But that's not the case. Through that I discovered that #18167 wasn't quite correct because it basically undoes the `decrement(skip)` behavior and by accident got all the tests passing. The real reason #18167 was passing its new test is that the related batch was flushed to completion (because `decrement(skip)` ran the flush again, because `skip` was `false`). But it's flawed as seen by the new `async-stale-derived-4` test. So I reverted part of #18167 and instead added logic to `decrement` to still run the batch if there is no more pending work left, or discard it if it's superseeded by the later batches. The other change is to skip processing a batch if it's not in `batches` anymore. For that I tweaked where a new one is added to the Set, and skipping just-created-batches during `batch.#commit()`
🦋 Changeset detectedLatest commit: 2d97120 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 |
|
|
No idea why tsgo suddenly fails, I suspect they had a release that either broke something or revealed type issues in our code. Either way unrelated to this PR |
|
The skipping logic isn't right, unfortunately. Here's a case that behaves correctly on (If you add a Ideally I think Meantime, I'll extract the uncontroversial bits of this PR |
|
Sounds like we will need something like #17971 after all - essentially we gotta track which things of a batch are superseeded by others, but these newer ones don't outright reject the async value, instead they tell the batch "hey you're superseeded" and only once all are superseeded will it be discarded and free up the others to resolve. Until then it has time to resolve the values itself.
I'm having a hard time imagining how honestly 😓 the Update: Found a way to get your example passing. We now tell the obsolete batch to resolve together with the latest subsequent batch that overlaps with it. |
|
The |
…rejecting (#18180) This incorporates some of the fixes and insights from #18177, but gets rid of the `skip` logic. Instead, we differentiate between _stale_ and _obsolete_ promises. A promise is stale if it has been overtaken by a subsequent update, and was rejected with `STALE_REACTION`: ```ts async function search(query: string) { return fetch(`/search?q=${query}`, { signal: getAbortSignal() }).then((r) => r.json()); } ``` In this case, if we start typing `pot`, and then finish typing `potato`, the first promise will eventually resolve with the results for `/search?q=potato`, instead of the batch entering a weird limbo/zombie state. A promise is obsolete if it belongs to a now-destroyed effect, meaning that toggling `show` doesn't result in an accumulation of never-resolving batches: ```svelte {#if show} {await neverResolves()} {/if} ``` Fixes part of sveltejs/kit#15431 --------- Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com> Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
|
Closing in favor of #18180 |
Fixes part of sveltejs/kit#15431
This contains two fixes which combined make one of the tests pass. While doing so I realized that #18167 wasn't quite correct (because the changes that are correct and made the test pass regressed others) so I adjusted that, too.
The first change is in
deriveds.js#async-derived, where we don't justbreakwhen we come across our own batch, because that assumes thatdeferredscontains batches in incrementing order. But that's not the case.Through that I discovered that #18167 wasn't quite correct because it basically undoes the
decrement(skip)behavior and by accident got all the tests passing. The real reason #18167 was passing its new test is that the related batch was flushed to completion (becausedecrement(skip)ran the flush again, becauseskipwasfalse). But it's flawed as seen by the newasync-stale-derived-4test. So I reverted part of #18167 and instead added logic todecrementto still run the batch if there is no more pending work left, or discard it if it's superseeded by the later batches.The other change is to skip processing a batch if it's not in
batchesanymore. For that I tweaked where a new one is added to the Set, and skipping just-created-batches duringbatch.#commit()