-
Notifications
You must be signed in to change notification settings - Fork 58
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
Change media type to 'application/graphql-response+json' #215
Conversation
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.
Merging this would be considered a breaking change, should that be reflected in graphql-http too (by releasing v2)? Or do we keep the deprecated application/graphql+json
around?
IMHO since graphql-http is a reference implementation, I wouldn't bloat it with history - all changes in the spec should be reflected 1:1.
Since this new media type now disambiguates between request and response, should we extend the |
@spawnia I considered that, but I don't think there's a need for that in v1 and I'd rather get it shipped. Adding it in v1.1 as a recommendation should be fine I think. @enisdenjo You published it yesterday, how many downloads do you have? I'd suggest just pulling it and releasing a patch release. But technically if you want to adhere strictly to semver it's v2. |
Not many downloads at all, I think we'd be a-ok releasing this change as a patch bump. But, if merging this PR takes time and v1 gets traction (I doubt), we'll just go with v2. |
@enisdenjo It is my opinion that, given that graphql-http is a "reference implementation," that it reflects the current status of the spec itself. The spec is in stage-1 (proposal) and changes are to be expected. The same would be true for the reference implementation. This brings up something important, though: The reference implementation should contain tags that map to the different versions of the spec. |
This begs the question whether I made a mistake by starting with However, it being a reference implementation would allow a bit of leeway in terms of semver. What I mean is that the spec compliance is a part of the library, abstracted away from the user, and spec breaking changes might not merit major bumps of the library.
Good point, I'll see what I can do. |
I think this is a great improvement in clarity. |
There seems to be six approvals for this and no dissent so I'm going to go ahead and merge it 👍 |
… to `application/graphql-response+json` As per graphql/graphql-over-http#215
Released in graphql-http@v1.3.0. Due to the low downloads count on npm and arguments in #215 (comment), I didn't bother making it a breaking change. |
graphql#215 related changes
This commit deprecates the `"application/graphql+json"` media type in favor of the new `"application/graphql-response+json"`, since the former has been removed in graphql/graphql-over-http#215. Closes gh-29617
As of graphql/graphql-over-http#215, the official media type for GraphQL HTTP responses is now `"application/graphql-response+json"` instead of `"application/graphql+json"`. The latter is now deprecated and support will be removed in the future. This commit now favors the new media type. HTTP Clients are still supposed to send requests with the `"application/json"` content type. Closes gh-563
Wait... so application/graphql-response+json is now used for requests 😵💫?! How's that an improvement in clarity? |
Requests still use |
Ah, ok, phew 😅 I misunderstood. Thanks a bunch for the quick response! |
@IvanGoncharov pointed out that
application/graphql
(or reallytext/graphql
) would make sense to use as the media type for GraphQL documents when stored in files (typically.graphql
files). To avoid confusion, we should change the media type this spec recommends. Looking at https://www.iana.org/assignments/media-types/media-types.xhtml it seems that adding-response
and-request
are common patterns, so I propose that we change the media type to 'application/graphql-response+json'CC @enisdenjo as this may impact graphql-http