-
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: Scalar serialize as built-in scalar type #326
Conversation
Currently, Custom Scalars only describe their name and no other behavior about them can be programmatically determined. However in practice most custom scalar types are specializations of one of the existing built-in scalar types (String, Int, Float, etc.). This proposes adding one additional piece of metadata to custom scalars, a "serializes as" type which must be a built-in scalar type. This serialize as type describes what kinds of values are allowed to be provided as input to that type and what kind of value is expected to serialize from that type. This metadata is useful for code-generation systems which expect to generate code in well typed environments. It can also improve the validation accuracy and IDE ergonomics by providing a similar level of information about custom scalars as exist for the built-in scalars. To provide this metadata to those kinds of tools, this also extends the introspection system to return the serialize as type in the `ofType` field.
What if I'm using a transport which supports additional types, like ordered maps or something, which aren't one of the built in types? Also, this wouldn't support the currently widely used I think this is a great proposal, but it's too restrictive if all scalars have to be serializable in this way. It would be awesome if it were optional, so you could get default type generation but only for the types that have this specified. |
@leebyron Great PR 👍
@stubailo According to this PR "serialize as" is fully optional:
So it doesn't introduce any breaking changes and you can still define custom scalar as previously, e.g. Without this restriction, GraphQL schema can have arbitrarily long chains of scalars like |
in general i like this addition, Though I have concerns about this part:
Do you think it would make sence to relax this rule and allow custom scalar values to be defined in terms of other custom scalar values? I do see that it can simplify this feature from client perspective, but there is still some value to be gained when server is able to describe nested constraints. That said, i just checked how I defined On the other hand, if we don't allow nested definitions, then i'm a buit lost. From this description:
I undestood that the whole point of this feature is to focus on the sematic meaning of the scalar value. By restricting it to only built-in scalars, we greatly deminish it's value, in my opinion (built-in scalars do not provide much of veriety). In this case I feel that it actually would be better to expose
Still not 100% about soundness of my argument. Would love to hear your thoughts on this :) |
Sorry, I totally misread! Thanks for the clarification @IvanGoncharov |
I think @IvanGoncharov made a good point above about this. Allowing custom scalars to be defined in terms of other custom scalars essentially produces a chain, where the end of that chain is either something that client tooling understands or something it does not. Ultimately that tooling needs to resolve the chain and determine if it has the knowledge to do anything more specific. The motivation for restricting to the built-in scalar types is that any other type a client tool won't know what to do with, and biasing towards a simpler solution protects us from incidental complexity in the future. But I'm definitely open to discussion on this. I would be curious to hear of an example where the nested constraints would be useful for solving a problem which would weigh in favor of a slightly more complicated solution.
I do think there are two related but different problems here. The one that drove this proposal is client-side tooling which seek to understand how to treat custom scalars. What should client tooling do with a A different but related problem is how to define custom scalars which actually expose new serialization behavior. For example a My opinion is that the first problem is more important to solve since client code-gen has become important for nearly all popular GraphQL client frameworks (Apollo, Relay, and FB's native apps), where defining custom scalars in terms of the literals they accept rather than how they serialize would not help. The second problem is interesting in its ability to allow GraphQL to grow to support more kinds of serialization behavior, but is not as much a problem encountered by clients now and to solve would likely require more than just describing the literals that type accepts. Is that a fair characterization? |
Yes, this sounds good! After thinking about it fo a while, I also think that it is a good idea to approach this from a practical perspective. Approaching it from a client and codegen perspective is quite helpful. At least for numbers, these tools need to know the actual size of a number, the literal information is not very helpful here. So even limited subset of semantic information is already quite helpful. This gives me another idea: should we distinguish between input and output base type? for a |
I know this is an early draft, but from the word-smithery perspective some of the wording here is a bit awkward. In a few places we have this structure:
The "which it serializes as" pattern is a bit of a mouthful and I suspect may be harder to read for English-as-a-second-language readers. I'd suggest reordering these to avoid the trailing "as". In the example above, for instance, we could say:
(Or similar.) Likewise:
could become:
|
As we discussed during the last WG meetup there was no major objection to this proposal. However, I want to raise my concern about SDL syntax. With the proposed change you will be able to define custom scalars in two ways:
I'm concerned that developers may use the first form (because it shorter) for the cases where the second form is the correct choice. Also, I think the majority of APIs either don't need scalar with custom serialization behavior or they use scalars provided by GraphQL implementations (e.g. Sangria's So there is an issue: the short form ( I expect the following example scenario:
I strongly believe that the syntax for defining scalars with the new serialization behavior should signalize (like I'm still not sure about how alternative syntax may look like but here are a few of the possible variants: custom SomeScalarType
custom scalar SomeScalarType
scalar SomeScalarType asCustom
scalar SomeScalarType as Custom
scalar SomeScalarType as Any Would be great if someone can come up with a better one. I think it is still possible to introduce such changes to the SDL because it's not merged into the spec yet and because Descriptions as strings PR also introduces breaking change. What do you think about it? P.S. @leebyron Would be great to discuss it before |
Is there any further movement on this @IvanGoncharov? I remember you championing this. 👑 |
@schickling Thanks for ping 👍 But I'm still concern about SDL problem I described in my previous comment and think it should be disscussed before SDL syntax will be set in stone. |
Currently, Custom Scalars only describe their name and no other behavior about them can be programmatically determined. However in practice most custom scalar types are specializations of one of the existing built-in scalar types (String, Int, Float, etc.).
This proposes adding one additional piece of metadata to custom scalars, a "serializes as" type which must be a built-in scalar type. This serialize as type describes what kinds of values are allowed to be provided as input to that type and what kind of value is expected to serialize from that type. This metadata is useful for code-generation systems which expect to generate code in well typed environments. It can also improve the validation accuracy and IDE ergonomics by providing a similar level of information about custom scalars as exist for the built-in scalars.
To provide this metadata to those kinds of tools, this also extends the introspection system to return the serialize as type in the
ofType
field.This spec text has a reference implementation here: graphql/graphql-js#914