-
Notifications
You must be signed in to change notification settings - Fork 59
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 support for GraphQL over HTTP media type #558
base: main
Are you sure you want to change the base?
Conversation
✅ Docs Preview ReadyNo new or changed pages found. |
✅ Deploy Preview for apollo-ios-docc canceled.
|
✅ Deploy Preview for eclectic-pie-88a2ba ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Done. |
|
||
// This test is odd because you might assume it would fail but there is no content-type checking on standard | ||
// GraphQL response parsing. So this test is here to ensure that existing behaviour does not change. | ||
func test__response__givenUnknownContentType_shouldNotFail() throws { |
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've added tests to keep the existing behaviour of standard GraphQL response parsing including this odd one.
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.
Tests all look good. I just thing that we should clean the repeated string constants up with a helper method.
@@ -59,7 +59,7 @@ struct MultipartResponseDeferParser: MultipartResponseSpecificationParser { | |||
for dataLine in chunk.components(separatedBy: Self.dataLineSeparator.description) { | |||
switch DataLine(dataLine.trimmingCharacters(in: .newlines)) { | |||
case let .contentHeader(type): | |||
guard type == "application/json" else { | |||
guard type == "application/graphql-response+json" || type == "application/json" else { |
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.
We should create a helper method for this. We're doing string comparisons against strings that are really constants. And we're doing the same comparison in two places. This is prone to future bugs.
We should store the constants as StaticString
s and have a helper method to check the content header.
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'll put this PR back into draft and make that change when I get back.
Closes apollographql/apollo-ios#3485.
application/json
. Instead it simply attempts to deserialize anything as if it is JSON. It's only in the multipart parsers that we do a content type check for the chunks. @AnthonyMDev - this hasn't been an issue for now but do you think we should add this check? I'm inclined to leave the standard parser as it is.