-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[RFC] Custom Scalar Specification URLs #649
Changes from all commits
5a2e3e3
2e24437
1d1cf95
22d2921
9a599f6
bc9a408
2c2057e
a620718
5522c9a
2ed5043
50b5510
e235752
42360e4
4b5a285
968f961
6c58094
bab4fab
038b6f8
eb18c03
96fb0d1
f8065c3
a2401b7
318b753
f2bf007
89014b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -150,6 +150,9 @@ type __Type { | |
|
||
# should be non-null for NON_NULL and LIST only, must be null for the others | ||
ofType: __Type | ||
|
||
# should be non-null for custom SCALAR only, must be null for the others | ||
specifiedBy: String | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It correlates to some extent with #300 . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sungam3r there are some experiments on introspection extensions. Technically you can already today extend introspection but this alway comes with the danger that you violate a future specification. @IvanGoncharov was experimenting on this one with GraphQL-js. At the moment you can put metadata on almost everything there. We do almost the same with hot chocolate. The next thing would be to allow to have something like extensions on introspection so that there is a point where you can cleanly extend the introspection types. Doing this through directives brings some challenges since directives, specifically their arguments are input types that could bear challenges to automatically extend the introspection from those. @IvanGoncharov maybe we should start a new discussion on that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a separate graphql-dotnet branch in which directives are already returned via introspection. We use this forked preview version in production.
I agree. It’s not scary for us. We need new features now, and not when they (possibly) appear in the specification. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sungam3r i did mean that more from them perspective. Hc allows extending the introspection already for a couple of versions. But the user that does that has to know that he/she might violate a future spec. We just have a warning in the docs that you can do it but may run into an issue in the future. |
||
} | ||
|
||
type __Field { | ||
|
@@ -238,13 +241,15 @@ actually valid. These kinds are listed in the `__TypeKind` enumeration. | |
|
||
Represents scalar types such as Int, String, and Boolean. Scalars cannot have fields. | ||
|
||
A GraphQL type designer should describe the data format and scalar coercion | ||
rules in the description field of any scalar. | ||
Also represents [Custom scalars](#sec-Scalars.Custom-Scalars) which may provide | ||
`specifiedBy` as a scalar specification URL. | ||
|
||
Fields | ||
|
||
* `kind` must return `__TypeKind.SCALAR`. | ||
* `name` must return a String. | ||
* `specifiedBy` may return a String (in the form of a URL) for custom scalars, | ||
otherwise must be {null}. | ||
* `description` may return a String or {null}. | ||
* All other fields must return {null}. | ||
|
||
|
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.
While implementing this in graphql/graphql-js#2276 @IvanGoncharov provided the following comment:
Question: Do we want to make that same change here as well? If so this likely applies to
URL
as well, and effects a few locations in this PR.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.
Point taken that it could become confusing, but it's not confusing yet, and there is definitely value in having a concrete example that lines up with use cases a lot of actual developers will run into. I'm planning to leave this as is, and we can change it if becomes an issue later.