Conversation
martinbonnin
left a comment
martinbonnin
left a comment
There was a problem hiding this comment.
Quick cursory review. Couple of comments but looks good!
spec/GraphQLOverHTTP.md
Outdated
| | `application/json` | An alternative type for responses (to support legacy clients) | | ||
| | Name | Description | | ||
| | ----------------------------------- | --------------------------------------- | | ||
| | `application/graphql-response+json` | The preferred type for server responses | |
There was a problem hiding this comment.
Does it still makes sense to use a table, now that there's only one row (same for the table for requests, above).
Also is "preferred" still the right word?
spec/GraphQLOverHTTP.md
Outdated
| | `application/graphql-response+json` | The preferred type for server responses | | ||
|
|
||
| Note: Servers MAY additionally support `application/json` as a response media | ||
| type. |
There was a problem hiding this comment.
Does it make sense to link somewhere that might specify what the behavior should be? (i.e. non-200x status codes unacceptable, etc.). We (the Apollo Client team) used this document to throw errors properly for application/json and non-compliant status codes, and with the removal here, that gets lost.
But maybe thats the point of this change? 🤔
There was a problem hiding this comment.
Would it be OK to put that as markdown file in this repo? https://github.com/graphql/graphql-over-http/LEGACY.md or something similar?
There was a problem hiding this comment.
I'm fine with that too! I'm mostly looking for it written down somewhere so that client libraries know what to expect.
There was a problem hiding this comment.
Yup, keeping that somewhere for reference would be greatly appreciated.
It would also be great if that document could show the q=0.9 as an example. I get why it's removed here, but implementers will need to know what to expect in real life, and some examples will keep the number of different usages down. Without examples, there will be all kinds of different strings floating around very soon.
|
@michaelstaib I took a stab at consistency and formatting there. I also added a You can browse it there: https://graphql-over-http.mbonnin.net/draft/#sec--application-json-response-media-type |
* Remove the security note, we have a non-normative note now * remove the table from the media types * Add compatibility non-normative note * Focus on JSON * Do not repeat ourselves * Be explicit that null or absent is equivalent to an empty map * Move Accept compatibility to compatibility section * no exception for the empty string * move POST null parameters outside of a note, this is normative * consistency * Move the "ignore unknown keys" above because it applies to both GET and POST * Move the 'null' references to the POST section. It doesn't make much sense for GET * move the GET requests MUST not be mutations above examples * Add another example for GET. Keep GET and POST sections symmetrical * 200 is not necessarily good? * move down * Add application/json media type * format * formating * Do not use uppercase in non-normative notes * Move all status code requirements to the status code section * Add an extra sentence to help with the reading flow
|
@michaelstaib I hear you like PRs so I merged my PR in your PR so we can fix conflicts from other PRs in this PR. I hope that's OK, let me know otherwise! |
|
I've not read this fully, but I like the idea of moving the legacy support to an appendix. Let's make it a literal appendix - a separate file. Let's make that appendix A and I can move persisted documents to appendix B. I'm excited for GraphQL-over-HTTP v1.1 🎉 |
|
@benjie sounds good 👍 There are lots of things in this PR. Let me try to break it down into smaller ones. Hopefully I can do that for today's wg 🤞 |
6 participants
No description provided.