fix(signals): surface sync rejection from thenables (closes #2764)#2783
Open
tsushanth wants to merge 2 commits into
Open
fix(signals): surface sync rejection from thenables (closes #2764)#2783tsushanth wants to merge 2 commits into
tsushanth wants to merge 2 commits into
Conversation
solidjs#2766) When `onSettled` is called from a children-forbidden scope (most commonly from inside another `onSettled` callback, which runs as a tracked effect), the implementation fell back to enqueueing a one-shot callback on the global queue and invoked the returned cleanup eagerly inside the same flush: globalQueue.enqueue(EFFECT_USER, () => { const cleanup = callback(); … cleanup?.(); }); That immediately tore down helpers whose intent was "install on settle, tear down on dispose" — composable helpers using `onSettled` internally saw their subscription set up and ripped down in the same tick (solidjs#2766). Capture the calling owner and, when the callback returns a function, attach it via `cleanup()` under that owner with `runWithOwner`. The cleanup now fires when the captured owner is disposed, matching the contract of the non-forbidden path. Orphaned helpers (no owner at all) drop the cleanup rather than run it immediately — there is no point at which firing it would be correct. Add a regression test in `tests/onSettled.test.ts` that wraps the reporter's pattern (a nested `onSettled` inside an outer `onSettled`) and asserts the subscription stays alive until `dispose()`.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2764.
When a thenable invokes its rejection handler synchronously during
.then(...), the error was being silently dropped. The handler'sif (!isSync) handleError(e)branch skipped the call during the sync window, and the subsequentif (!resolved)branch unconditionally threwNotReadyError— so the boundary observed pending forever and never reachedErrored.The fix mirrors the success handler's sync/async split for rejections: capture the error inside the sync window, then after
.thenreturns, surface it throughhandleErrorand rethrow so the calling computation propagates it to the nearest error boundary.Test
Added a regression test in
tests/syncThenable.test.tsthat wraps a synchronously-rejecting thenable in acreateMemoundercreateErrorBoundaryand asserts the boundary receives the error.Without the fix: test times out / boundary stays pending.
With the fix: boundary catches the error.