-
Notifications
You must be signed in to change notification settings - Fork 738
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
feature: Adds request Accept
header
#3167
Conversation
@@ -70,6 +71,10 @@ public extension GraphQLOperation { | |||
return nil | |||
} | |||
|
|||
static var hasDeferredFragments: Bool { | |||
false |
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 will get set for each operation that has deferred fragments in it as part of the work for #3114.
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.
Looks good, just have a question about deferred fragments in subscriptions.
) | ||
} | ||
|
||
if Operation.hasDeferredFragments { |
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.
Is there a chance that we will have a subscription that also has deferred fragments? This would override that protocol spec in that case.
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.
No, I don't think defer is supported in subscriptions. In that case we'd be indirectly relying on graphql-js to invalidate the operation.
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.
Is graphql-js already handling that case?
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 went looking for validation of my assumption and it was not as easy to find as I thought it was. I did find reference to not being allowed on a Subscription in the spec edits PR though.
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.
As long as it's definitely not allowed in the specification, I'm fine with merging this then. If there is a bug in the future, that is a bug in GraphQL-js not Apollo-iOS. :)
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're fine. IMO defer on subscriptions is pointless.
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.
OK, more confirmation in Apollo Client's httpLink.
Closes #3143
Related to #3146 (introduces the
MultipartResponseDeferParser
)This PR ensures that when an operation is recognized as having deferred fragments the request automatically has the correct
Accept
HTTP header set to specify a multipart response and one that matches the currently configured defer spec parser.