-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Editorial: Define type definitions #870
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
base: main
Are you sure you want to change the base?
Conversation
possible types as the *concrete Object type* during field execution. | ||
|
||
|
||
### Wrapped Types |
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.
@benjie - would love your thoughts on this newly written subsection.
This is based on your comment with very minor changes:
- Wrapped type - the kind of type including Lists and Non-Nulls
- List type - changed from your suggestion of List wrapped type
- Non-Null type - changed from your suggestion of Non-Null wrapped type
- Inner type - the type immediately wrapped (eg the
ofType
)- Item type - I added this - the inner type of a List
- Nullable type - I added this - the inner type of a Non-Null
|
||
GraphQL supports two abstract types: interfaces and unions. | ||
### Leaf and Composite Types |
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.
Good time to clarify #826 .
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.
Thanks for making this connection! That earlier phrasing is definitely confusing. I'll weigh in on that issue and make sure this PR covers the solution.
In writing this change I found a number of cases where we confuse "value" and "type" and interchange them even in the same paragraph. We've also made the mistake of using "scalar" when we mean "leaf".
@leebyron I think some unrelated changes have made their way into this PR, e.g. |
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.
These changes really improve the clarity of the text IMO - excellent work!
spec/Section 3 -- Type System.md
Outdated
|
||
:: A *composite type* compose together leaf types and other composite types as a |
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.
:: A *composite type* compose together leaf types and other composite types as a | |
:: A *composite type* composes together leaf types and/or other composite types as a |
Above we defined "There are two kinds of leaf types: Scalar type and Enum type." However technically lists/non-null of these are also leaf types - but the text doesn't make it clear if these are included under the definition of "leaf type" or not. This technicality becomes important here when we're talking about mixing leaf types and composite types to make a new type since here it's essential that the wrapped types are factored in. I'm not sure how best to address this.
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.
A leaf type is scalar/enum, not including wrapped. Same with composite types being the set described here, not including wrapped.
This is good feedback on this paragraph being falsely precise - since we can compose leaf or composite types (leaf + composite = named) and any wrapped of either of those (named + wrapped(named) = all types). Thats just all types.
Going to replace with something simple:
A composite type composes other types together as a set of named fields
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.
Sounds good to me 👍
types depending on how the wrapped type may be used. | ||
uses categorize types as an *input type* or an *output type*. Some kinds of | ||
types, like Scalar and Enum types, can be used as both input types and output | ||
types; other kinds of types can only be used in one or the other. |
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.
A little off-topic Whilst we're on this subject... generally scalar
types are suitable for input and output, but in some cases (for example the Upload
scalar used by the GraphQL Upload Spec) a custom scalar may only be suitable for one but not the other. Is this specifically disallowed, or do we need to be more cautious with our wording?
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.
We actually don't have the tools to assert this restriction right now. We don't specifically disallow it, but we also can't enforce it (with validation - a custom scalar could just fail coercion, but thats a bit of a hack IMO)
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.
Yep; I’m happy keeping the status quo; just felt it was worth mentioning.
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.
We actually don't have the tools to assert this restriction right now
I'd be interested in digging into this distinction for a later release: I have some usages around input vs output enums that I'd like to add clarity to. As the spec stands, this statement:
GraphQL services must return one of the defined set of possible values. If a reasonable coercion is not possible they must raise a field error.
can mean it's actually a breaking change to add a new enum value to an existing enum type (we make no statement about avoiding exhaustive switches on output enums, for example). We don't get into details in the spec itself on what a breaking type system change is, with just a short blurb about type system evolution.
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.
Relevant: graphql/graphql-js#968 (comment)
I have now completed the review but have no further feedback 👍 |
5d0b38c
to
3494138
Compare
e5d586a
to
0c8de00
Compare
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define *kinds* of types. In doing so applies the definitions arrived at in #845 for "wrapped types". Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
0c8de00
to
be692fe
Compare
✔️ Deploy Preview for graphql-spec-draft ready! 🔨 Explore the source changes: e0820c8 🔍 Inspect the deploy log: https://app.netlify.com/sites/graphql-spec-draft/deploys/61b72d3b6b79800007eb25c7 😎 Browse the preview: https://deploy-preview-870--graphql-spec-draft.netlify.app |
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.
Quite a lot of these are suggestions rather than strict change requests. I think this is significantly clearer than the previous spec text - and given it's taken me over an hour review it I can only imagine how long it took to write it - excellent work Lee!
There are three composite *output type*: Object, Interface, and Union, and one | ||
composite *input type*: Input Object. |
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.
When defining leaf type
above we linked to the Scalar type
/ Enum type
- we should do so here also.
There are three composite *output type*: Object, Interface, and Union, and one | |
composite *input type*: Input Object. | |
There are three composite *output type*: *Object type*, *Interface type*, and *Union type*, and one | |
composite *input type*: *Input Object type*. |
:: A *composite type* composes other types together as a set of named fields, | ||
allowing the composition of arbitrary type hierarchies and nested trees of data. |
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.
Do you feel this accurately describes a Union? I'm on the fence. Maybe "resulting in a set of named fields" to make it clear it's not always directly a set of named fields? I'm also fine if we don't change this.
The *Object type* is the concrete output *composite type* which describes an | ||
actual concrete value which a field will resolve to during execution, composed | ||
of other *output type* fields, allowing responses of nested data. |
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.
Something about this wording doesn't feel quite right, but I cannot figure out how to rewrite it. My concerns relate primarily to lists - e.g. a field might not resolve to this value directly, but it may be included in the result returned. Also I'm not sure about the "a field will resolve to" because it kind of implies that all fields resolve to object types, or that there's always at least one field that resolves to an object type. Maybe "a field may resolve to", or "a field with an Object type result type would resolve to".
An *Interface type* defines a list of fields. Any type which implements an | ||
interface must implement those fields. Whenever a field claims it will | ||
return an *Interface type*, it will return an implementing | ||
*concrete Object type* during field execution. | ||
|
||
A *Union type* defines a list of possible types. Similar to Interfaces, | ||
whenever a field claims it will return a *Union type*, it will return one of the | ||
possible types as the *concrete Object type* during field execution. |
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 isn't wrong, but the wording "whenever a field claims it will return a X type" doesn't cover the case of lists/nested lists. Not sure if that should be factored in?
* Let {typeA} be the *nullable type* of {typeA} | ||
* Let {typeB} be the *nullable type* of {typeB} | ||
* If {typeA} or {typeB} is a *List type*. | ||
* If {typeA} or {typeB} is not a *List type*, return false. | ||
* Let {typeA} be the *item type* of {typeA} | ||
* Let {typeB} be the *item type* of {typeB} |
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.
Added periods.
* Let {typeA} be the *nullable type* of {typeA} | |
* Let {typeB} be the *nullable type* of {typeB} | |
* If {typeA} or {typeB} is a *List type*. | |
* If {typeA} or {typeB} is not a *List type*, return false. | |
* Let {typeA} be the *item type* of {typeA} | |
* Let {typeB} be the *item type* of {typeB} | |
* Let {typeA} be the *nullable type* of {typeA}. | |
* Let {typeB} be the *nullable type* of {typeB}. | |
* If {typeA} or {typeB} is a *List type*. | |
* If {typeA} or {typeB} is not a *List type*, return false. | |
* Let {typeA} be the *item type* of {typeA}. | |
* Let {typeB} be the *item type* of {typeB}. |
* If {locationType} is a *non-null type*: | ||
* If {variableType} is NOT a *non-null type*, return {false}. |
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.
* If {locationType} is a *non-null type*: | |
* If {variableType} is NOT a *non-null type*, return {false}. | |
* If {locationType} is a *Non-Null type*: | |
* If {variableType} is NOT a *Non-Null type*, return {false}. |
* Return {AreTypesCompatible(nullableVariableType, nullableLocationType)}. | ||
* Otherwise, if {variableType} is a non-null type: | ||
* Otherwise, if {variableType} is a *non-null type*: |
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.
* Otherwise, if {variableType} is a *non-null type*: | |
* Otherwise, if {variableType} is a *Non-Null type*: |
* Otherwise, if {locationType} is a *list type*: | ||
* If {variableType} is NOT a *list type*, return {false}. |
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.
* Otherwise, if {locationType} is a *list type*: | |
* If {variableType} is NOT a *list type*, return {false}. | |
* Otherwise, if {locationType} is a *List type*: | |
* If {variableType} is NOT a *List type*, return {false}. |
* Return {AreTypesCompatible(itemVariableType, itemLocationType)}. | ||
* Otherwise, if {variableType} is a list type, return {false}. | ||
* Otherwise, if {variableType} is a *list type*, return {false}. |
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.
* Otherwise, if {variableType} is a *list type*, return {false}. | |
* Otherwise, if {variableType} is a *List type*, return {false}. |
This broad change uses definition phrases to define each type, as well as expands the set of sub-sections at the top of the "Types" section which define kinds of types. In doing so applies the definitions arrived at in #845 for "wrapped types".
Makes editorial improvements throughout, removing duplicate language or attempting to improve correctness or legibility.
Closes #826
Closes #845