Skip to content

Fixup combined error#874

Merged
udnay merged 1 commit into
mainfrom
fixup-combined-error
Jul 10, 2025
Merged

Fixup combined error#874
udnay merged 1 commit into
mainfrom
fixup-combined-error

Conversation

@udnay

@udnay udnay commented Jul 9, 2025

Copy link
Copy Markdown
Contributor

Small change to how we format a combined error to iterate through the GrahpQL errors and append the network error.

Changelog

[no-changelog-required]

@udnay

udnay commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

This change is part of the following stack:

Change managed by git-spice.

@udnay udnay force-pushed the fixup-combined-error branch 3 times, most recently from 802d3df to bca3bec Compare July 9, 2025 17:23
@udnay udnay requested a review from airhorns July 9, 2025 17:49
Comment thread packages/api-client-core/src/support.ts Outdated
if (response.error.networkError?.message) {
response.error.message = `[Network] ${response.error.networkError.message}`;
} else {
response.error.message = `[Network] No message, stack: ${String(response.error.networkError.stack)}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stringify the whole error error here instead of relying on just the stack. In JS you can throw anything, like throw "foo", so the error may not have a message or a stack and just be a string. Since we dont exactly know what it is I think it might be wise to be defensive and just stringify the result right out

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah kk, my test was just returning Error with no stack when I did string(response.networkError) which is why I explicitly grabbed the stack. Would something like
[Network] No message, error: string(response.error.networkError) \nstack: ${String(response.error.networkError.stack)} work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

{
operation: null as any,
data: null,
error: new CombinedError({ networkError: [new Error("foo"), new Error("foo")] as any }),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm this almost seems on purpose

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's why I wasn't sure. Based on the types it can't be an array but we're pretty adamant about that. It does not seem like an array of network errors is possible

@udnay udnay force-pushed the fixup-combined-error branch from bca3bec to 4f149b7 Compare July 9, 2025 20:54
@udnay

udnay commented Jul 9, 2025

Copy link
Copy Markdown
Contributor Author

Updated, good to merge on 🍏 ?

@udnay udnay force-pushed the fixup-combined-error branch from 4f149b7 to 122a5cc Compare July 9, 2025 21:01
@udnay udnay force-pushed the fixup-combined-error branch from 122a5cc to 55e5da1 Compare July 10, 2025 01:19
@udnay udnay merged commit cd0b33f into main Jul 10, 2025
10 checks passed
@udnay udnay deleted the fixup-combined-error branch July 10, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants