fix: $effect.pending sends updates to incorrect boundary
#16729
Conversation
🦋 Changeset detectedLatest commit: 2fc7880 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 |
|
| this.pending = true; | ||
| // we intentionally do not try to find the nearest pending boundary. If this boundary has one, we'll render it on reset | ||
| // but it would be really weird to show the parent's boundary on a child reset. | ||
| this.pending = this.has_pending_snippet(); |
There was a problem hiding this comment.
Don't think this is quite right, since this uses the getter which will set the parent to true if the current one doesn't have a pending snippet. Did you mean to do this?
| this.pending = this.has_pending_snippet(); | |
| this.#pending = this.has_pending_snippet(); |
|
|
||
| /** | ||
| * Whether this boundary is inside a boundary (including this one) that's showing a pending snippet. | ||
| * Derived from {@link props.pending} and {@link cascading_pending_count}. |
There was a problem hiding this comment.
That "derived from ..." part is confusing because that's not what's happening.
| if (this.has_pending_snippet()) { | ||
| this.#pending = value; | ||
| } else if (this.parent) { | ||
| this.parent.pending = value; |
There was a problem hiding this comment.
The set pending logic feels extremely suspicious to me. In all cases where we're setting it to true, it's because the boundary in question has a pending snippet. Why would we ever want to set a parent boundary's pending state to false? If we comment out this line, no test fails. Just all feels wrong
|
|
||
| /** | ||
| * Whether this boundary is inside a boundary (including this one) that's showing a pending snippet. | ||
| * Derived from {@link props.pending} and {@link cascading_pending_count}. |
There was a problem hiding this comment.
what is the @link syntax meant to do?
| this.#pending_count += d; | ||
| /** @param {number} d */ | ||
| #update_cascading_pending_count(d) { | ||
| this.#cascading_pending_count = Math.max(this.#cascading_pending_count + d, 0); |
There was a problem hiding this comment.
huh? this can't possibly be right
| this.#cascading_pending_count = Math.max(this.#cascading_pending_count + d, 0); | |
| this.#cascading_pending_count += d; |
3 participants
Prior to this PR, all pending count updates were forwarded to the nearest parent boundary with a
pendingsnippet. This made$effect.pendingincorrect if you had nested boundaries where an inner boundary didn't have apendingsnippet.This corrects the behavior and clarifies the meaning of a few concepts in the boundary code:
pendingnow explicitly means "this boundary is inside of a boundary (including this one) that is currently trying to render a pending snippet"#pending_countnow directly corresponds to$effect.pendingfor this boundary#cascading_pending_countis the same but treats boundaries with nopendingsnippet as porous, which means pending snippet management can be managed separately from$effect.pendingmanagementresetcorrectly manages the#cascading_pending_countand also explicitly only renders the pending snippet of the boundary being reset, not any of its parentsget_pending_snippetis dead. Code can now callget_snippetand usependingorupdate_pending_countand it will just do the right thingBefore 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