Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
a6ff322 to
9a74fd8
Compare
This attempts to clarify some naming conventions in the spec and makes
one meaningful syntax change.
This changes `SchemaDefinition` to have a zero-or-more list of
`OperationTypeDefinition` rather than one-or-more. It removes some language
that refers to this "at least one" requirement as well.
Why? Because schema can be extended with `SchemaExtension`. This brings
this into consistency with other elements of the type definition
language where definitions allow providing zero items for aspects that
require one at the point of schema validation. This was overlooked
before since most people use the default names, but it is not required
to do so, and sometimes you need to supply the schema definition for
explicitness, but are within the context of a single file that is
a subset of a full schema where other operation types are defined elsewhere.
Specifically, this example used to not parse, and now should:
```graphql
schema {}
```
What has not changed is that schema are required to provide a query root
type. A schema without this operation type will not pass validation, and
would later need something like this to become valid:
```graphql
extend schema { query: MyQueryType }
```
The bulk of this change edits naming of some concepts in an attempt to
improve clarity:
- Renames `OperationType` to `OperationKind`. This was already
inconsistently used throughout the spec doc, but now is consistently
using "kind". This avoids overloading the term "type".
- Renames `RootOperationTypeDefinition` to `OperationTypeDefinition`. We use "root" in an overloaded way and this was redundant. This improves the text to be a well defined provider of the type for a root selection
set of an operation, so this shorter name is less redundant and easier
to read. This also renames "default root type name" to "default operation type name" for consistency.
- Replaces "root field" with "root selection set". A root field is
a holdover concept from a 10yr old version of GraphQL. More accurately
today these are "fields on the root selection set". This makes the
latter well defined. I searched for "root" to make sure it refers only
to a well defined "root selection set". This our last and only
remaining use of the term "root".
9a74fd8 to
4f792d2
Compare
Implements the changes proposed in graphql/graphql-spec#1015
…that defines it. - Rephrase root fields as "collected from the root selection set" to be clear that the special bit is the result of CollectFields and remove ambiguity around the subscriptions single field constraint. - Fixed an inconsistency where ResolveFieldEventStream used `rootValue` instead of `initialValue` like the rest of the document. - Some minor copy tweaks and issues caught
2d8e495 to
e546c74
Compare
benjie
left a comment
There was a problem hiding this comment.
I like this change in general, but I have one concern...
In English, "type" can very easily be interpreted to mean the same as "kind", in fact "kind" shows up as part of the definition of "type" on Dictionary.com.
So having "operation type" and "kind of operation" as distinct concepts when they feel the same to a casual reader doesn't sit quite right with me.
I'm in favour of using "root type" rather than "operation type" - I think "root type" and "root selection set" pair nicely together, and this removes the above confusion.
| :: Each operation is represented by an optional operation name and a _root | ||
| selection set_ which describes the initial set of fields to request from that | ||
| _operation type_. |
There was a problem hiding this comment.
The "initial" here is perhaps a little confusing; it implies that we'll request the fields on this selection set first, and then there'll be a later in which more fields are requested; that's not right since the fields are gathered recursively, then all requested together in parallel (except for mutations of course). But with the word "initial" deleted it's not quite right either, because it doesn't fully describe the fields without traversing any fragment spreads. Not sure what it should say though 😓
| ### Subscription Operation Definitions | ||
|
|
||
| #### Single Root Field | ||
| #### Single collected root field |
There was a problem hiding this comment.
| #### Single collected root field | |
| #### Single Collected Root Field |
(It would definitely be nice to have an auto-linter for this!)
| must provide the operation name as described in {GetOperation()}. | ||
| Note: While each subscription must have exactly one field collected from the | ||
| _root selection set_, a document may contain any number of subscription | ||
| operations, each of which may contain different fields. When executed, a |
There was a problem hiding this comment.
Since we now state "subscription" operation, it should be singular and arguably we should be clear that the same field is also allowed.
| operations, each of which may contain different fields. When executed, a | |
| operations, each of which may contain the same or a different field. When executed, a |
| If all fields from the source of the field error up to the _root selection set_ | ||
| of the operation return `Non-Null` types, then the {"data"} entry in the | ||
| response should be {null}. |
There was a problem hiding this comment.
Do we need the word "up" here? It makes me question which way is up or down on a "tree" - especially since GraphQL trees go left to right 😆
| If all fields from the source of the field error up to the _root selection set_ | |
| of the operation return `Non-Null` types, then the {"data"} entry in the | |
| response should be {null}. | |
| If all fields from the source of the field error to the _root selection set_ | |
| of the operation return `Non-Null` types, then the {"data"} entry in the | |
| response should be {null}. |
There was a problem hiding this comment.
Also... I think this isn't actually correct:
type Query {
a: [B]!
}
type B {
throw: Int!
}
query {
a { throw }
}All fields (throw, a) from the source of the field error (throw) return Non-Null types, and yet this should not result in the response being null.
Happy to raise this in a follow up PR instead if you prefer since the error already existed.
|
The argument for this change has a symmetric argument for allowing empty type and interface definitions, a la: |
This attempts to clarify some naming conventions in the spec and makes one meaningful syntax change.
This changes
SchemaDefinitionto have a zero-or-more list ofOperationTypeDefinitionrather than one-or-more. It removes some language that refers to this "at least one" requirement as well.Why? Because schema can be extended with
SchemaExtension. This brings this into consistency with other elements of the type definition language where definitions allow providing zero items for aspects that require one at the point of schema validation. This was overlookedbefore since most people use the default names, but it is not required to do so, and sometimes you need to supply the schema definition for explicitness, but are within the context of a single file that is a subset of a full schema where other operation types are defined elsewhere.
Specifically, this example used to not parse, and now should:
schema {}What has not changed is that schema are required to provide a query root type. A schema without this operation type will not pass validation, and would later need something like this to become valid:
The bulk of this change edits naming of some concepts in an attempt to improve clarity:
Renames
OperationTypetoOperationKind. This was already inconsistently used throughout the spec doc, but now is consistently using "kind". This avoids overloading the term "type".Renames
RootOperationTypeDefinitiontoOperationTypeDefinition. We use "root" in an overloaded way and this was redundant. This improves the text to be a well defined provider of the type for a root selection set of an operation, so this shorter name is less redundant and easier to read. This also renames "default root type name" to "default operation type name" for consistency.Replaces "root field" with "root selection set". A root field is a holdover concept from a 10yr old version of GraphQL. More accurately today these are "fields on the root selection set". This makes the latter well defined. I searched for "root" to make sure it refers only to a well defined "root selection set". This our last and only remaining use of the term "root".
Minor editorial change where we only use a the form
{`query`}when referring to the actual keyword rather than the concept