-
Notifications
You must be signed in to change notification settings - Fork 2
feat(graphql): query verifies the response #767
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
Conversation
KarlMae
left a comment
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.
While testing I converted some services to use the generic and it's so much cleaner, thanks!
I left some improvement ideas.
| if (graphQLResponse.errors && graphQLResponse.errors.length > 0) { | ||
| throw this.extractGraphQLError(graphQLResponse); | ||
| } | ||
| return graphQLResponse.data as T; |
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.
IMO the function would read much smoother like this:
let response = await fetch(`https://${this.apiServer}/graphql`, init);
let responseJson = await response.json();
if (responseJson.errors?.length) {
throw this.extractGraphQLError(responseJson);
}
if (!responseJson.data) {
throw new Error(`Server response is not valid GraphQL response. Response: ${ JSON.stringify(responseJson) }`);
}
return responseJson.data as T;
It subjectively has an easier to follow structure:
- Fetch
- Check for errors
- Check for data and unwrap it
The casting is also not necessary?
let graphQLResponse: GraphqlResponse = responseJson;
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.
Casting is still needed but I refactored it a bit
| const query = queryToString(queryDocument); | ||
| ): Promise<T> { | ||
| const query = print(queryDocument); | ||
| if (!query || query.length === 0) { |
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 can remove this check as well.
Query can not be undefined as it is clearly set on the line above.
Query can also not have length 0 because gql DocumentNode can not be empty as tested here:
DOM
<button (click)="handleClick()">Test</button>
class
async handleClick(): Promise<void> {
await Qminder.GraphQL.query(gql``);
}
Result
Screen.Recording.2025-01-06.at.10.31.25.mov
KarlMae
left a comment
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.
Let's go 🚀
Breaking change how
Qminder.GraphQL.queryworks.stringas an argument. Have to usegqlfromgraphql-tagdatain the responsedatafrom unwrapped responseExample usage is in
app.ts