-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Enable 'schema' keyword to be provided without root operations #1166
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
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
🚢
What about instead allowing the block to be empty? i.e.:
This is perhaps less likely to break existing parsers |
It's not. import { parse } from 'graphql';
const ast = parse(`schema {}`);
console.dir(ast);
To me, an empty block would explicitly say there are no operation types, whereas a missing block says to use the defaults. |
This was discussed at the WG at the beginning of this month, Lee had some pushback: https://youtu.be/Lo0OhLoMBII?t=2754 If you have a strong use case for this, please do comment. |
Extracted the bugfix part to #1167 |
I think I'm generally in agreement with Lee's pushback that: """
This is an example schema
"""
schema
type Query {
a: Int
} is less clear than """
This is an example schema
"""
schema {
query: Query
}
type Query {
a: Int
} And since a) I have a lot of open PRs, and b) we don't need this for disabling error propagation (because we've moved that to "service" instead of "schema") I'm going to close it temporarily. I think if schema directives become common in future I would like to revisit it because I prefer: +schema @newBehavior
type Query {
a: Int
} over +schema @newBehavior {
+ query: Query
+}
type Query {
a: Int
} But we'll cross that bridge when we get to it. |
This PR is motivated by:
In the above PR we want to be able to indicate the error propagation behavior of a schema. However, I find it displeasing that to do so we would need to also explicitly detail the operations supported by the schema when they would normally be auto-detected based on their names:
This PR makes it so that you can use a schema keyword to add directives and/or description to the schema without needing to also specify the root operation types if they can be inferred by the default naming. The above diff would thus become:
Much nicer! ✨
This is also useful without the
onError
feature, since it allows you to apply a description to a schema without having to detail the operations:PR #1164 applies these changes onto #1163; but this PR is mergeable into the spec as-is.