-
Notifications
You must be signed in to change notification settings - Fork 737
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
Add extensions property to GraphQLResult. #1370
Add extensions property to GraphQLResult. #1370
Conversation
- Updated references. - Implemented tests in ParseQueryResponseTests.
@paulkite: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
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.
Hi! Sorry I was on vacation last week. Mostly looks good, just one thought.
Sources/Apollo/GraphQLResponse.swift
Outdated
@@ -72,13 +75,16 @@ public final class GraphQLResponse<Data: GraphQLSelectionSet> { | |||
let data = try decode(selectionSet: Data.self, | |||
from: dataEntry, | |||
variables: variables) | |||
let extensions = body["extensions"] as? JSONObject |
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.
This should probably be outside the if let
statement since theoretically someone could return stuff in extensions
even if data
is nil
.
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.
Ah. Yeah, I was wondering if I was making the right choice there or not. I'll make an update ASAP. Thank you!
- Moved reference to extensions object into function level scope to be able to assign it to the result even if data doesn't exist in the response. - Updated tests.
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.
🎉 Thanks for the PR!
Addresses issue: 1348