Skip to content

Improve clarity of scalar types #597

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

Merged
merged 2 commits into from
Jan 10, 2020
Merged

Improve clarity of scalar types #597

merged 2 commits into from
Jan 10, 2020

Conversation

leebyron
Copy link
Collaborator

@leebyron leebyron commented Jul 23, 2019

Attempts a more holistic fix to #535 which unifies language around built-in scalars and serialization, shortening the intro to this section in the process.

@leebyron leebyron added the ✏️ Editorial PR is non-normative or does not influence implementation label Jul 23, 2019
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

@leebyron Definitely big improvement over previous text.

@andimarek
Copy link
Contributor

Honestly I still find this section confusing/unclear regarding some details. Let me try to explain:

"Internal types" is used without being clearly explained, but I think it is implicitly clear that these are the types used by the engine at execution time.

Then we have two different kind of "serialization" steps happening: one is the result coercion which maps the resolved value to a serialized value for a specific scalar. This happens during the execution. Then we have a second serialization step which is described in the "Response" section and this describes how the whole result map/tree can be serialized for example to JSON. But it also makes it clear that this serialization is not clearly defined and GraphQL can be serialized to anything as long this format support List,Map,String, null primitives.

So to summarize for graphql-js with JSON transport serialization we have the following table:

GraphQL Scalar type internal type JSON transport type
Int number number
Float number number
Boolean boolean boolean
String string string
ID string string
Map Object Object
List Array Array

For Java with JSON this table looks like this:

GraphQL Scalar type internal type JSON transport type
Int int number
Float float number
Boolean boolean boolean
String string string
ID string string
Map Map Object
List List Array

I think we must more clearly seperate between transport serialization (which is outside of the GraphQL spec for anything except JSON) and serialization of an resolved value to an internal type (result coercion). As disccussed separetely with @leebyron scalars should probably only be coerced to primitive internal types (or recursivley Map/List of primitives).

One challenge with that restriction is that the result coersion is not observable from the outside: for example a custom scalar DateTime could theorecitally be coerced to internal DateTime object and then transport serialized to a String in JSON.

To summarize:

  • We should make more clear that result coercion and transport serialization are two different things
  • we should clearly define what internal types are and how they map to primitive GraphQL types (aka build-in scalar). This means defining the term "primitive internal types"
  • make clear that custom scalars are only be allowed to be coerced into primitive internal types (even if it might not obervable)

@IvanGoncharov
Copy link
Member

we should clearly define what internal types are and how they map to primitive GraphQL types (aka build-in scalar). This means defining the term "primitive internal types"

make clear that custom scalars are only be allowed to be coerced into primitive internal types (even if it might not obervable)

@andimarek I'm opposed to it since it will restrict the ability to take advantage of protocol-specific types. For example, gRPC supports file transfers and it could be represented by File, Blob, etc. custom scalars and I don't think binary streams will fit your definition of primitive types.

@andimarek
Copy link
Contributor

@IvanGoncharov that is the interesting part: you are right: File etc would not fit. The fundamental problem with this scalars is: what can you expect from them? How File would be transport serialized? You have to know about it in order to deal with it, right?

Maybe it is more accurate to say that cross platform scalars needs (should) only be mapped to primitive internal types? Because if you don't you can't really specify any cross platform behaviour.

@IvanGoncharov
Copy link
Member

The fundamental problem with this scalars is: what can you expect from them? How File would be transport serialized? You have to know about it in order to deal with it, right?

For example, using the same mechanism we discussed by for DateTime. Or by writing "GraphQL over gRPC spec".

Maybe it is more accurate to say that cross platform scalars needs (should) only be mapped to primitive internal types? Because if you don't you can't really specify any cross-platform behavior.

If the underlying protocol is cross-platform it means scalars are also cross-platform.

Practically speaking from my experience with graphql-js it's enough to have parse and serialize callbacks to make custom scalars as usable as standard scalars.
Can you please give some example of functionality that is impossible to implement without proposed restrictions?

BTW. I think BigInt is an even better example of a custom scalar and it's a standard type in some languages (JS & Java) but is non-standard in many others. At the same time, it's natively supported by JSON since it doesn't have any limitation on the maximum and minimum numbers.

@andimarek
Copy link
Contributor

Let me be a bit more detailed about what I mean:

In order to use a scalar (lets make our life easier here and just talk about scalars used as output) correctly you need to know what comes back. You query a field foo of type MyScalar: what does it mean?

There are two different possible ways how to define MyScalar clearly:

Using primitive internal types

By describing the coerced (serialized step 1) result using a primitive internal type. Meaning MyScalar maps to an primitve internal type which is semantically equivalent to one the GraphQL Types String, Int, Float, Boolean, or a Map/List of it. For example you define MyScalar as serialized to String in RFC 3339 format. Then you are done. Because you are piggybacking on the existing well known and clearly defines types of the build-in scalar you don't need to care about the transport serialization at all. MyScalar maps to String and String maps for JSON to JSON String. If you use another transport serialization you need to define anyway how the primitive types are mapped, meaning MyScalar will be covered automatically.

Using an non primitive type

Lets assume MyScalar is not coerced into a non primitive type. Lets say further we want to map it into an internal type representing DateTime but it doesn't have to be String. Then you also need to answer the question how to transport serialize it, because how should the users know what is returned? Of course you can do that and you could say: for JSON it is serialized as String in RFC 3339. But then why not serialized it clearly by coercing it into a String?
If you use a non primitive type to represent your scalar you also need to specify at least the JSON serialization. For example BigInt you mentioned: it is not clear how BigInt is serialzed in JSON. It can be number or String.
The other practical problem with custom scalars not returning primitive types: you can't control the transport serialization from the custom scalar implementation alone. If I write a scalar which return s BigInt in Java I also need somehow make sure that it is transport serialized as String or whatever I choose. This is tricky to say the least.

Of course you can go down this route but then you have to to do exactly what you mentioned that you need a an extra "serialize this Scalar with this protocol spec".

@IvanGoncharov
Copy link
Member

I think the current version of this PR is a big improvement as is.
Also in current form, it's strictly editorial change but limiting scalars only to primitive types is not.
@leebyron @andimarek Can we merge this PR as is and have a discussion about restricting scalars to primitive types in the separate issue?

@eapache
Copy link
Member

eapache commented Oct 17, 2019

Agree with Ivan that this PR is a step in the right direction and should not be held up by the larger question.

My two cents on the larger question: I think the talk of two separate serialization steps is a mistake, and likely not intended. Result coercion is not related to serialization IMO, it is a convenience for resolver authors. As such, scalars should not be required to serialize to one of the spec primitives, but they should be required to specify serialization behaviour in some transport(s), and their use in other transports should be considered undefined.

Attempts a more holistic fix to #535 which unifies language around built-in scalars and serialization, shortening the intro to this section in the process.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants