Skip to content

Conversation

leebyron
Copy link
Collaborator

@leebyron leebyron commented Feb 9, 2023

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:

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:

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".

  • Minor editorial change where we only use a the form {`query`} when referring to the actual keyword rather than the concept

@leebyron leebyron added 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) ✏️ Editorial PR is non-normative or does not influence implementation labels Feb 9, 2023
@netlify
Copy link

netlify bot commented Feb 9, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit e546c74
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/63e566a4e8ee73000833c7aa
😎 Deploy Preview https://deploy-preview-1015--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@leebyron leebyron changed the title RFC: Root Types clarity RFC: "Root" Types clarity → "Operation Type" Feb 9, 2023
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".
leebyron added a commit to graphql/graphql-js that referenced this pull request Feb 9, 2023
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
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +294 to +296
:: 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_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😓

@@ -251,11 +251,11 @@ query getName {

### Subscription Operation Definitions

#### Single Root Field
#### Single collected root field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title case:

Suggested change
#### 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we now state "subscription" operation, it should be singular and arguably we should be clear that the same field is also allowed.

Suggested change
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

Comment on lines +806 to +808
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}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆

Suggested change
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}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mjmahone
Copy link
Contributor

mjmahone commented Mar 2, 2023

The argument for this change has a symmetric argument for allowing empty type and interface definitions, a la:

type MyType {}
interface MyInterface {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Editorial PR is non-normative or does not influence implementation 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants