-
Notifications
You must be signed in to change notification settings - Fork 318
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
chore(ai): add graphql errors to useAIGeneration #5900
Conversation
🦋 Changeset detectedLatest commit: 724244b 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 |
if (errors) { | ||
hasError = true; | ||
message = errors.map((error) => error.message).join(' '); | ||
} | ||
|
||
return [{ ...result, data, hasError, message }, handler]; |
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.
Can we ever get into a case where useDataState's result has an error message
and we also have graphqlErrors? In that case graphqlErrors would override that useDataState
message?
Though I think that would only happen in the case where our previous call had graphqlErrors, then our next generation resulted in an error
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.
I think we should be fine because the amplify data client (using appsync) will always return errors in the same way. I don't think there is another way to get an error from the data client.
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.
Thinking useDataState
is the wrong approach here for a handful of reasons:
output.hasError
istrue
when the API call itself fails, which is why it'sfalse
handled in this use caseoutput.message
exposes the error message of the failed API call- afaict
handleGenerate
is setting a subscription in a fire and forget fashion rather than using continuous calls to theuseDataState
handler to provide statefuldata
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.
🚢
I forgot my approval counts for nothing 😅 |
data, | ||
messages: errors, |
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.
Late question and asking mostly out of curiosity, how do consumers correlate data
and errors
?
Description of changes
Errors in GraphQL for AI generation routes are currently being sent to the client in the
errors
part of the response, but theuseAIGeneration
hook wasn't setting thehasError
. I also moved the error to themessage
part of the hook. It seemed weird that there could be different places to check for the error message?Issue #, if available
Description of how you validated changes
Checklist
yarn test
passes and tests are updated/addeddocs
,e2e
,examples
, or other private packages.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.