Skip to content
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

Merged
merged 1 commit into from
Aug 16, 2022
Merged

Conversation

benjie
Copy link
Member

@benjie benjie commented Aug 5, 2022

@IvanGoncharov pointed out that application/graphql (or really text/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

Copy link
Member

@enisdenjo enisdenjo left a 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.

@spawnia
Copy link
Member

spawnia commented Aug 5, 2022

Since this new media type now disambiguates between request and response, should we extend the officially recognized GraphQL media types to designate using the JSON encoding for GraphQL requests with application/graphql-request+json?

@benjie
Copy link
Member Author

benjie commented Aug 5, 2022

@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.

@enisdenjo
Copy link
Member

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.

@ghmcadams
Copy link
Collaborator

@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.

@enisdenjo
Copy link
Member

@ghmcadams

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 begs the question whether I made a mistake by starting with v1, and not v1-rc.1 and reserving the v1 release for the official v1 release of the spec. I did this intentionally to represent the state of the library, not the spec - the library is fully functional and itself worth being v1.

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.

The reference implementation should contain tags that map to the different versions of the spec.

Good point, I'll see what I can do.

@glasser
Copy link
Contributor

glasser commented Aug 8, 2022

I think this is a great improvement in clarity.

@benjie
Copy link
Member Author

benjie commented Aug 16, 2022

There seems to be six approvals for this and no dissent so I'm going to go ahead and merge it 👍

@benjie benjie merged commit 37f5d2c into main Aug 16, 2022
@benjie benjie deleted the media-types branch August 16, 2022 10:24
enisdenjo added a commit to graphql/graphql-http that referenced this pull request Aug 16, 2022
@enisdenjo
Copy link
Member

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.

sungam3r added a commit to sungam3r/graphql-over-http that referenced this pull request Sep 5, 2022
bclozel added a commit to spring-projects/spring-framework that referenced this pull request Dec 2, 2022
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
bclozel added a commit to spring-projects/spring-graphql that referenced this pull request Dec 5, 2022
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
@kaqqao
Copy link

kaqqao commented Dec 26, 2022

I think this is a great improvement in clarity.

Wait... so application/graphql-response+json is now used for requests 😵‍💫?! How's that an improvement in clarity?

@enisdenjo
Copy link
Member

Requests still use application/json, only the responses have the application/graphql-response+json content type.

@kaqqao
Copy link

kaqqao commented Dec 26, 2022

Ah, ok, phew 😅 I misunderstood. Thanks a bunch for the quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants