-
Notifications
You must be signed in to change notification settings - Fork 1.2k
add initial outline for field extensions #1196
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
|
|
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
martinbonnin
left a comment
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 looking into this! Few comments below
| - extend field MemberCoordinate Directives[const]? | ||
| - extend field description MemberCoordinate |
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.
Was the intent to have the ability to add a description to fields? The other extensions don't allow that. What happens if the field already has a description? I suggest we only allow adding directives:
| - extend field MemberCoordinate Directives[const]? | |
| - extend field description MemberCoordinate | |
| - extend field MemberCoordinate Directives[Const] |
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 was suggested by @benjie on Discord. I think it makes sense, especially in an AI age where descriptions give valuable info to LLMs. But true it needs validation rules.
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 the heads up 👍.
Interesting! Although I would find it a bit odd to allow adding descriptions on fields but not on types and more (maybe this should go to a different PR that does that more broadly and keep this one focused on directives).
If we do want to keep this I think the grammar should look like:
| - extend field MemberCoordinate Directives[const]? | |
| - extend field description MemberCoordinate | |
| - extend field MemberCoordinate Directives[Const] | |
| - extend field Description MemberCoordinate |
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'd be happy extending extend type to allow a prefixed description. The rule would be if it already has a description it would be invalid, just like adding the same interface twice.
Option 1:
"Description"
extend type MyType
Option 2:
extend type "Description" MyType
To me, the former is preferred over the latter, even though both are suboptimal.
There's also option 3:
extend "Description" type MyType
Or, and this may be the best option, option 4:
describe type MyType "Description"
describe field MyType.field "Description"
describe argument MyType.field(arg:) "Description"
describe enum MyEnum "Description"
describe enumValue MyEnum.VALUE "Description"
describe directive @foo "Description"
describe argument @foo(arg:) "Description"
Would mean the description is separate from the rest of the extension, but I think this is fine.
Thanks for highlighting this @BoD
benjie
left a comment
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 editorial
| FieldExtension : | ||
|
|
||
| - extend field MemberCoordinate Directives[Const] | ||
| - extend field Description MemberCoordinate |
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.
Description is contentious, let's remove it for now.
| - extend field Description MemberCoordinate |
| FieldExtension : | ||
|
|
||
| - extend field MemberCoordinate Directives[Const] | ||
| - extend field Description MemberCoordinate |
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.
| - extend field Description MemberCoordinate |
| - extend field Description MemberCoordinate | ||
|
|
||
| Field extensions are used to represent a field which has been extended from some | ||
| previously defined field. For example this may be a GraphQL service which is |
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.
| previously defined field. For example this may be a GraphQL service which is | |
| previously defined field. For example this may be used by a GraphQL service which is |
| In this example, we can deprecate the id field on the User type. | ||
|
|
||
| ```graphql example | ||
| extend field User.name @deprecated(”Some reason”) |
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.
Beware of typographic quotes!
| extend field User.name @deprecated(”Some reason”) | |
| extend field User.name @deprecated(reason: "Some reason") |
| extend field User.name @deprecated(”Some reason”) | ||
| ``` | ||
|
|
||
| ** Field Validation ** |
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.
The "Type" here relates to "type" as in "type-system" rather than type as in "object type".
| ** Field Validation ** | |
| **Type Validation** |
|
|
||
| ** Field Validation ** | ||
|
|
||
| Field validation have the potential to be invalid if incorrectly defined. |
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.
| Field validation have the potential to be invalid if incorrectly defined. | |
| Fields have the potential to be invalid if incorrectly defined. |
| 1. MemberCoordinate must be resolved to an existing field on a object or | ||
| interface 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.
| 1. MemberCoordinate must be resolved to an existing field on a object or | |
| interface type. | |
| 1. MemberCoordinate must resolve to an existing field on a object or | |
| interface type. |
https://github.com/graphql/graphql-wg/blob/main/rfcs/FieldExtensions.md
#1162