RECOMMENDED to SHOULD - #416
Conversation
| execution. Using `405` (Method Not Allowed) is RECOMMENDED. This restriction is | ||
| necessary to conform with the long-established semantics of safe methods within | ||
| HTTP. | ||
| execution; status code `405` (Method Not Allowed) is RECOMMENDED. This |
There was a problem hiding this comment.
I've left this RECOMMENDED in place, it's the only one, but writing the sentence to use both MUST and SHOULD read really awkwardly and I believe RFC2119 gives us a choice of words to allow us to avoid these kinds of linguistic shenanigans.
| to add additional information to a request they MUST do so via other means; the | ||
| RECOMMENDED approach is to add an implementer-scoped entry to the {extensions} | ||
| object. | ||
| to add additional information to a request they MUST do so via other means, for | ||
| example by adding an implementer-scoped entry to the {extensions} object. |
There was a problem hiding this comment.
Adding to extensions is the "recommended" approach; but if you choose to set it via headers I wouldn't say you "should not" have done that. So I've lowered this to an example.
There was a problem hiding this comment.
This read better with RECOMMENDED.
A possible direction might be to point out that this is one of the reasons the extensions object exists and link to the spec for more informations. That would be more informative to users new to the spec.
b337df2 to
d4094f1
Compare
d4094f1 to
d38c825
Compare
d38c825 to
89f6e06
Compare
martinbonnin
left a comment
martinbonnin
left a comment
There was a problem hiding this comment.
The RECOMMENDED wording grew on me. I've spent a couple of years with this spec using RECOMMENDED and it worked fine by me. All in all no strong opposition but also no strong drive to change this. I'll abstain.
89f6e06 to
36f8f38
Compare
alf
left a comment
alf
left a comment
There was a problem hiding this comment.
I have no strong opinions, both before and after looks good to me.
| It is a common convention to end the path component of the URL with `/graphql`, | ||
| for example: |
There was a problem hiding this comment.
A lot of graphql security hardening guides argue against using /graphql. My take is that if you're relying on the fact that your endpoint is non-standard for security then you're at a bad place.
Still, I don't really see the benefit of recommending the path component. While it's all about discoverability for developers/tooling, it isn't something we can rely on. Saying that this is a common convention is better than recommending that implementers follow the convention. Good change.
| If it is not known that the server supports `application/graphql-response+json`, | ||
| it is RECOMMENDED that the client set the `Accept` header to | ||
| the client SHOULD set the `Accept` header to | ||
| `application/graphql-response+json, application/json;q=0.9`. |
There was a problem hiding this comment.
Why shouldn't the client set the Accept header to application/graphql-response+json, application/json;q=0.9 if it is known that the server supports application/graphql-response+json? 😸
There was a problem hiding this comment.
If it knows the server supports the desired media type, why Accept a worse one when you know it should never be used?
| to add additional information to a request they MUST do so via other means; the | ||
| RECOMMENDED approach is to add an implementer-scoped entry to the {extensions} | ||
| object. | ||
| to add additional information to a request they MUST do so via other means, for | ||
| example by adding an implementer-scoped entry to the {extensions} object. |
There was a problem hiding this comment.
This read better with RECOMMENDED.
A possible direction might be to point out that this is one of the reasons the extensions object exists and link to the spec for more informations. That would be more informative to users new to the spec.
5 participants
Before merging this, merge:
I am not particularly in favor of this change; but if we're going to do it we should do it right... I have put some normative changes in this, for example previously we RECOMMENDED that URLs end with
/graphqlbut I'm not willing to say that they SHOULD end with/graphql(even though they have the same weight in RFC2119) so I've noted that it's a common convention instead.I would recommend that people create their GraphQL APIs at
/graphqlor similar. Doing so is recommended.For someone who has deployed their GraphQL API at
https://example.com/apiorhttps://graphql.example.comI would not say that they "SHOULD NOT" have done that. It's fine.