fix(solid-query): ignore error if data undefined so can redirect #9841
Conversation
🦋 Changeset detectedLatest commit: a6069f5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
View your CI Pipeline Execution ↗ for commit a6069f5
☁️ Nx Cloud last updated this comment at |
WalkthroughUpdates error handling logic in server-side subscription of useBaseQuery to require data presence before rejecting errors. Also adds a corresponding changelog entry documenting the patch bump and behavior change for @tanstack/solid-query. Changes
Sequence Diagram(s)sequenceDiagram
participant Sub as Server Subscription
participant Handler as Error Handler
participant Result as Result State
Note over Sub,Result: Error handling comparison
rect rgb(200, 220, 255)
Note over Handler: OLD: Any error → reject
Sub->>Handler: unwrappedResult has error
Handler->>Result: Reject immediately
end
rect rgb(220, 255, 220)
Note over Handler: NEW: Requires result.data
Sub->>Handler: Check if result.data exists
alt data exists
Handler->>Result: Reject with error
else data undefined
Handler->>Result: Resolve with hydrated result
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9841 +/- ##
===========================================
+ Coverage 45.74% 76.39% +30.65%
===========================================
Files 200 19 -181
Lines 8404 394 -8010
Branches 1919 81 -1838
===========================================
- Hits 3844 301 -3543
+ Misses 4112 76 -4036
+ Partials 448 17 -431
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/green-lands-occur.md (1)
1-5: Expand the changeset description to explain the behavioral change.The current description is too brief for a behavioral change in error handling. Consider adding:
- The specific use case (redirect scenarios with undefined data)
- That this affects server-side hydration/SSR path
- Potential implications (errors with undefined data won't reject during SSR)
Apply this diff to improve the description:
--- '@tanstack/solid-query': patch --- -Skip errors if returned data is undefined +Skip errors during SSR when returned data is undefined. This allows redirect responses that return undefined to avoid triggering error rejection in server-side query hydration, enabling proper handling of redirects in frameworks like TanStack Router for Solid.packages/solid-query/src/useBaseQuery.ts (1)
177-187: Consider documenting the asymmetry between server and client error handling.The client subscriber (lines 177-187) has fundamentally different error handling than the server subscriber (lines 148-168):
- Server: Uses promises with reject/resolve
- Client: Updates state reactively without rejecting
While this asymmetry may be intentional due to SSR vs. client hydration differences, it would be helpful to document why they differ, especially since your change only affects the server path.
Consider adding a comment explaining the design:
const createClientSubscriber = () => { + // Client-side errors are handled through reactive state updates + // rather than promise rejection, allowing the UI to respond + // to error states without throwing const obs = observer() return obs.subscribe((result) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/green-lands-occur.md(1 hunks)packages/solid-query/src/useBaseQuery.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/solid-query/src/useBaseQuery.ts (2)
248-259: Inconsistent error handling between lifecycle phases confirmed.The inconsistency you identified is real and significant. The code handles errors differently depending on when they occur:
- Line 258 (initial effect): Rejects errors immediately if
shouldThrowError()returns true, regardless of data state- Line 160 (subscription callback): Only rejects errors if
result.data !== undefinedThis creates unpredictable behavior: an error that occurs early might be rejected, but the same error arriving via subscription later could be swallowed if data is still undefined.
Recommended verification:
- Confirm whether errors should consistently consider data presence across both phases
- Test scenarios where errors occur during initial fetch vs. during refetch
- Determine if line 258 should also require
observerResult.data !== undefinedbefore rejecting
148-168: Verify whether error handling fallback at lines 248-259 adequately addresses the error masking concern and clarify redirect handling intent.The concerns about error masking and asymmetry are partially valid. The code at line 159 does skip rejection when
result.data === undefined && unwrappedResult.isError. However, there's a fallback error check at lines 248-259 that rejects ifobserverResult.isError && !observerResult.isFetching && !isRestoring() && shouldThrowError().Key questions to clarify:
Does the fallback catch masked errors? The fallback depends on state updates and timing — if errors during
data === undefinedupdateobserverResult.isError, they'll eventually be caught. But this creates a race condition window where the promise resolves before the error is thrown.What is the redirect scenario? The logic assumes redirects result in
data === undefined + isError === true. Is there explicit redirect detection, or is this a heuristic?Is asymmetry intentional?
createServerSubscriberhas data guard logic, butcreateClientSubscriberdoesn't handle the promise — instead relying on the fallback. The design rationale isn't documented in comments.Without understanding the redirect handling requirements, this appears to be either a legitimate workaround with undocumented assumptions or an incomplete error handling approach.
| const unwrappedResult = hydratableObserverResult(query, result) | ||
|
|
||
| if (unwrappedResult.isError) { | ||
| if (result.data !== undefined && unwrappedResult.isError) { |
There was a problem hiding this comment.
this only skips it for the server subscriber. I am not sure if there is a better heuristic here instead of just data is undefined
2 participants
🎯 Changes
When doing something like this in tanstack router for solid we get an error that the query can not be undefined. It seems like it should allow it to be undefined and even skip the error if its a redirect.
I'm not 100% this is the right fix but at least its a start.
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Release Notes