-
Notifications
You must be signed in to change notification settings - Fork 64
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
Remove watershed and prefer new format #330
base: main
Are you sure you want to change the base?
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.
This looks good to me.
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 like!
Co-authored-by: Shane Krueger <[email protected]>
Can we merge this one? I'd like to propose other changes but I'd rather not open too many PRs in parallel. |
I hacked it out quite quickly and didn't have time to proof-read it, I'm not sure it's currently consistent? |
LGTM. I find that it improves the readability of the spec compared to the current state and I'd be in favor of merging it. |
This is an alternative fix to #327 and #322 that does away with the watershed entirely. I'm coming to believe the watershed was a mistake.
We:
Accept
header (whilst noting that servers may support non-compliant clients).application/graphql-response+json
in theirAccept
headerapplication/json
a "legacy server"application/graphql-response+json
application/json
if and only if they want to support legacy (now non-compliant) clientsIt is currently a draft because I've not had time to fully vet it, just wanted to get my thoughts down - I wouldn't review the specific wording too deeply yet.