-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SDK: Throw errors when present in JSON responses #22666
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 990cbcf 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 comment was marked as resolved.
This comment was marked as resolved.
sdk/src/utils/extract-data.ts
Outdated
@@ -11,7 +11,7 @@ export async function extractData(response: unknown) { | |||
|
|||
if (type?.startsWith('application/json') || type?.startsWith('application/health+json')) { | |||
const result = await response.json(); | |||
if (!response.ok) throw result; | |||
if (!response.ok || 'errors' in result) throw result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tricky bit here is that a GraphQL response can have both errors
and data
at the same time. This solution would mean we would throw a partial-successful response as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh okay, that makes sense! How about something like the updated version? Using a proxy still allows the result to be returned as an object, and doesn't pollute it with extra keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever idea!
I'm a little conflicted on what I personally would expect to happen in this case. GraphQL is a bit of an odd duck in that you can have a "failed successfully" state. I'm not too familiar with Proxies yet myself; am I correctly reading that you'd basically access the errors through a virtual .apiErrors
property on the result? So you'd get
const result = []; // result came from SDK
result.apiErrors
// => GraphQL `errors` array if it exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's exactly right! And it would return undefined
if there were no errors, allowing you to check for any partial or complete failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm interesting! I'm wondering if that's expected enough for people to discover 🤔 I personally wouldn't expect to be able to find the errors on a "fake" property on an array, .. though I also don't have any better alternatives.
Is this something we want to make consistent with every way you fetch data? @br41nslug remind me, are we currently throwing an error on failed rest requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay yeah that's fair! But the returned data should be an object, no? For example the query
query {
items {
id
}
}
would return
{ items: [{ id: "123" }] }
but if an error is thrown, currently the result will be
{ items: null }
with no way to access the error. My proposal is to add the virtual field as a way to access any errors returned from the server.
At least this is my understanding of how things currently work, it's totally possible that I'm mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In GraphQL you're absolutely correct. I'm mostly wondering in what ways we can keep it consistent with the expected output types in REST and WebSocket use as well—and if that's even something we want to keep consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh okay, gotcha! Makes sense. I've been working with GraphQL and that's my use case, but yeah definitely would want to keep things consistent across the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we currently throwing an error on failed rest requests?
Yep we do https://github.com/directus/directus/blob/main/sdk/src/utils/request.ts#L24-L25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking in on this. I can confirm that errors encountered when using REST are correctly thrown before returning the proxied result, and are present in the thrown data. This should only apply to requests that failed when using GraphQL.
Scope
What's changed:
Potential Risks / Drawbacks
Review Notes / Questions
Fixes #22671