RFC: Define custom scalars in terms of built-in scalars. #914

leebyron · 2017-06-16T01:42:08Z

This proposes an additive change which allows custom scalars to be defined in terms of the built-in scalars. The motivation is for client-side code generators to understand how to map between the GraphQL type system and a native type system. As an example, a URL custom type may be defined in terms of the built-in scalar String. It could define additional serialization and parsing logic, however client tools can know to treat URL values as String. Presently, we do this by defining these mappings manually on the client, which is difficult to scale, or by giving up and making no assumptions of how the custom types serialize.

Another real use case of giving client tools this information is GraphiQL: this change will allow GraphiQL to show more useful errors when a literal of an incorrect kind is provided to a custom scalar. Currently GraphiQL simply accepts all values.

To accomplish this, this proposes adding the following:

A new property when defining GraphQLScalarType (ofType) which asserts that only built-in scalar types are provided.
A second type coercion to guarantee to a client that the serialized values match the ofType.
Delegating the parseLiteral and parseValue functions to those in ofType (this enables downstream validation / GraphiQL features)
Exposing ofType in the introspection system, and consuming that introspection in buildClientSchema.
Adding optional syntax to the SDL, and consuming that in buildASTSchema and extendSchema as well as in schemaPrinter.
Adding a case to findBreakingChanges which looks for a scalar's ofType changing.

leebyron · 2017-06-16T01:42:43Z

If there's general agreement, I'll be following up with more test coverage as well as the minor spec text change to cover the introspection addition.

stubailo · 2017-06-16T02:57:41Z

Can I add another wish list item here? I don't know if it's specifically relevant to this PR.

It would be really nice to be able to override the serialization and deserialization of built-in types in GraphQL-JS. Specifically, I always run into this with ID and MongoDB. It's super convenient to just be able to return a Mongo ObjectID which is already kind of like a string, but it's not possible to use the scalar serialization feature and the ID type at the same time. So you end up with awkward wrapper code, or you have to define a custom MongoID scalar.

leebyron · 2017-06-16T07:07:50Z

I think that's out of scope here but happy to discuss in another issue or pr

jjergus · 2017-06-16T15:57:34Z

src/language/__tests__/schema-kitchen-sink.graphql

@@ -41,6 +41,8 @@ union AnnotatedUnionTwo @onUnion = | A | B

 scalar CustomScalar

+scalar StringEncodedCustomScalar = String


bikeshedding:

= doesn't seem like the appropriate operator here because they're not equal, but one is a subset of the other

maybe a different operator like : or a keyword like as or of?

Yeah, I agree. implements seems like the closest thing, : seems reasonable as well

@dschafer also suggested : IIRC. My preference for = was based on matching the rest of the SDL forms, but I agree that the implication of equality isn't correct.

I'm a bit worried that : is also overloaded - we most use it for defining the types or values to arguments but also aliases.

Maybe a keyword is best to make this as literate as possible, we rejected : in favor of implements for the same reason when defining object types. I kind of like as as short for "serializes as". Thoughts from others?

rwe · 2017-07-31T14:49:33Z

@leebyron — An issue that seems directly relevant to this RFC is validation and scalar compatibility. Sometimes we'll want to transition a scalar field from "String" to something more descriptive, like "ID" or custom "DateTime" scalar. For example:

type Query {
  foos(ids: [String!]): [Foo]
}

to:

type Query {
  foos(ids: [ID!]): [Foo]
}

Unfortunately, client queries which refer to that field as String fail to validate, so either there needs to be a breaking change, or perform type transitions by designating new field/argument names—even though the actual JSON input variables or response are not changing.

# This old query would fail to validate after the otherwise-benign schema change,
# and there is no solution currently short of updating all clients in lockstep,
# or adding a new argument name.
query($myFooIds: [String!]) {
  foos(ids: $myFooIds) { bar }
}

Would this PR help with that kind of transition? For example, would it make sense to allow input fields whose scalars serialize to compatible JSON to emit warnings on type mismatches, rather than a validation error?

asaf-romano · 2017-08-04T05:48:09Z

Will this change also help with "asymmetric" scalars? For example, in our system we've a Token type which is (roughly) btoa(JSON.stringify(value) in its serialized form and also in client inputs (literal or value), but a simple JS object on the server side. In other words, parseValue and parseLiteral work on strings, whereas serialize works with objects.

leebyron

Some thoughts after coming back to this

leebyron · 2017-12-20T23:52:23Z

src/type/definition.js

@@ -478,12 +480,14 @@ export class GraphQLScalarType {
  // Serializes an internal value to include in a response.
  serialize(value: mixed): mixed {
    const serializer = this._scalarConfig.serialize;
-    return serializer(value);
+    const serialized = serializer(value);
+    return this.ofType ? this.ofType.serialize(serialized) : serialized;


This effectively chains the serialize functions together if more than one are provided. Should we consider having the same chaining behavior for parsing input values as well?

Agreed 👍

leebyron · 2017-12-20T23:58:15Z

src/type/validate.js

+  context: SchemaValidationContext,
+  scalarType: GraphQLScalarType,
+): void {
+  if (scalarType.ofType && !isSpecifiedScalarType(scalarType.ofType)) {


I wonder if this is over-constrained. Perhaps it would be okay to allow a scalar to serialize in terms of any other scalar type?

My thought is that people add custom scalars for two reasons. First is to represent some higher level concept like "Email" or "PhoneNumber" where "serializes as String" is useful. Second is to represent a new kind of transport primitive like "BigInt" where they definitionally serialize as something new. However that first kind of higher-level concept scalar might want to say it serializes in terms of another non-spec scalar!

In other words - what makes the specification of a scalar special to warrant this limitation?

I believe we originally settled on this limitation to avoid needing to follow a chain of "ofType" to determine the final actual serialization type, but I think that might not be such a big deal

I believe we originally settled on this limitation to avoid needing to follow a chain of "ofType" to determine the final actual serialization type, but I think that might not be such a big deal

I think that decision about allowing recursive types should be done in the scope of the entire spec since it affects the entire type system. For example, you couldn't query all information about a type using __type anymore:

{ __type(name: "LocalPath") { name # ... ofType { name # ... # <= you can't construct a query that gets arbitrary long chain of `ofType` } } }

Or the fact that you can introduce loops inside types, so we need to make buildASTSchema, buildClientSchema and others loop-resistant. E.g.:

scalar A as C scalar B as A scalar C as B

I think allowing to better describe scalars isn't worth added complexity. However, if the similar changes are done in other places of type-system (e.g. graphql/graphql-spec#295) then the price is already paid and we could definitely add recursiveness to scalar types.

However that first kind of higher-level concept scalar might want to say it serializes in terms of another non-spec scalar!

We can allow specifying scalar serializable as any other scalar without as in the definition.

scalar BigInt scalar Counter as BigInt # Valid scalar UserCounter as Counter # Invalid

But with current terminology and SDL syntax, it would be very confusing 😞
That's why I think we need to separate custom scalar types and scalar types:

custom scalar BigInt scalar Counter as BigInt # Valid scalar UserCounter as Counter # Invalid

So we could say that scalar is serializable only as a standard scalar or a custom scalar.

I think this PR in its current form solves >80% of use-cases so we can release it as is and relax rules afterward.

leebyron · 2017-12-20T23:59:45Z

src/language/ast.js

@@ -448,6 +448,7 @@ export type ScalarTypeDefinitionNode = {
  +loc?: Location,
  +description?: StringValueNode,
  +name: NameNode,
+  +type?: NamedTypeNode,


I think this key should be more descriptive. A type having a field called "type" is confusing

Fully agree. How about serializableAs?

@leebyron Given current syntax scalar Url as String.
How about asType?

leebyron · 2017-12-21T00:01:38Z

@asaf-romano - that would still work fine - the internal representation as an object would first go through your custom scalar's serialize function before going through the serializing type's. That guarantees the result of the query will be a string, but still gives control over the internal representation

leebyron · 2017-12-21T00:02:39Z

Additional tests should be added to ensure validation works as expected

IvanGoncharov · 2017-12-21T16:43:51Z

@leebyron I rebased this PR on top of #1175 and working on validation tests and other suggestions.
Should I create separate PR or commit on top of this one?

IvanGoncharov · 2018-02-05T20:01:39Z

@leebyron I rebased this PR on top of master and squshed first 3 PRs (make it easier to rebase).
And I working on addressing review comments and adding more tests as a separate commit.
Should I push it into this PR or create a new one?

This proposes an additive change which allows custom scalars to be defined in terms of the built-in scalars. The motivation is for client-side code generators to understand how to map between the GraphQL type system and a native type system. As an example, a `URL` custom type may be defined in terms of the built-in scalar `String`. It could define additional serialization and parsing logic, however client tools can know to treat `URL` values as `String`. Presently, we do this by defining these mappings manually on the client, which is difficult to scale, or by giving up and making no assumptions of how the custom types serialize. Another real use case of giving client tools this information is GraphiQL: this change will allow GraphiQL to show more useful errors when a literal of an incorrect kind is provided to a custom scalar. Currently GraphiQL simply accepts all values. To accomplish this, this proposes adding the following: * A new property when defining `GraphQLScalarType` (`ofType`) which asserts that only built-in scalar types are provided. * A second type coercion to guarantee to a client that the serialized values match the `ofType`. * Delegating the `parseLiteral` and `parseValue` functions to those in `ofType` (this enables downstream validation / GraphiQL features) * Exposing `ofType` in the introspection system, and consuming that introspection in `buildClientSchema`. * Adding optional syntax to the SDL, and consuming that in `buildASTSchema` and `extendSchema` as well as in `schemaPrinter`. * Adding a case to `findBreakingChanges` which looks for a scalar's ofType changing.

IvanGoncharov · 2018-09-20T16:53:48Z

@leebyron I rebased this PR on top of master. I hope you don't mind?

IvanGoncharov · 2018-10-14T06:23:50Z

As suggested by @leebyron during last WG I'm adopting this PR as #1552

facebook-github-bot added the CLA Signed label Jun 16, 2017

leebyron force-pushed the schema-scalar-of-type branch from 528f8fb to 0a2aec0 Compare June 16, 2017 01:53

leebyron mentioned this pull request Jun 16, 2017

Add support for Scalar aliases graphql/graphql-spec#308

Closed

jjergus approved these changes Jun 16, 2017

View reviewed changes

leebyron closed this in 279f97f Jun 19, 2017

leebyron reopened this Jun 19, 2017

leebyron force-pushed the schema-scalar-of-type branch 2 times, most recently from 85a0203 to c9b713f Compare June 19, 2017 19:47

leebyron mentioned this pull request Jun 19, 2017

RFC: Scalar serialize as built-in scalar type graphql/graphql-spec#326

Closed

IvanGoncharov mentioned this pull request Oct 27, 2017

Add "Status of Scalar serialize as built-in scalar type RFC" to agenda graphql/graphql-wg#35

Merged

brysgo mentioned this pull request Nov 15, 2017

RFC: WithMetadata modifier #1089

Closed

vladar mentioned this pull request Dec 12, 2017

Allow objects with __toString in IDType webonyx/graphql-php#210

Merged

IvanGoncharov mentioned this pull request Dec 20, 2017

Rebase of "Schema scalar of type" #1173

Closed

leebyron force-pushed the schema-scalar-of-type branch from c9b713f to 637ee10 Compare December 20, 2017 23:47

leebyron commented Dec 20, 2017

View reviewed changes

IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label May 17, 2018

sibelius mentioned this pull request Sep 13, 2018

feat(scalar): add generics to scalars #1522

Closed

IvanGoncharov force-pushed the schema-scalar-of-type branch from 637ee10 to a0cf9b2 Compare September 20, 2018 16:51

IvanGoncharov mentioned this pull request Oct 14, 2018

RFC: Define custom scalars in terms of built-in scalars. #1552

Open

IvanGoncharov closed this Oct 14, 2018

IvanGoncharov deleted the schema-scalar-of-type branch October 14, 2018 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Define custom scalars in terms of built-in scalars. #914

RFC: Define custom scalars in terms of built-in scalars. #914

leebyron commented Jun 16, 2017 •

edited

leebyron commented Jun 16, 2017

stubailo commented Jun 16, 2017

leebyron commented Jun 16, 2017

jjergus Jun 16, 2017

stubailo Jun 16, 2017

leebyron Jun 16, 2017 •

edited

rwe commented Jul 31, 2017 •

edited

asaf-romano commented Aug 4, 2017

leebyron left a comment

leebyron Dec 20, 2017

IvanGoncharov Dec 21, 2017

leebyron Dec 20, 2017

IvanGoncharov Dec 21, 2017 •

edited

leebyron Dec 20, 2017

IvanGoncharov Dec 21, 2017

IvanGoncharov Sep 20, 2018

leebyron commented Dec 21, 2017

leebyron commented Dec 21, 2017

IvanGoncharov commented Dec 21, 2017

IvanGoncharov commented Feb 5, 2018 •

edited

IvanGoncharov commented Sep 20, 2018

IvanGoncharov commented Oct 14, 2018

		@@ -41,6 +41,8 @@ union AnnotatedUnionTwo @onUnion = \| A \| B

		scalar CustomScalar

		scalar StringEncodedCustomScalar = String

RFC: Define custom scalars in terms of built-in scalars. #914

RFC: Define custom scalars in terms of built-in scalars. #914

Conversation

leebyron commented Jun 16, 2017 • edited

leebyron commented Jun 16, 2017

stubailo commented Jun 16, 2017

leebyron commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron Jun 16, 2017 • edited

Choose a reason for hiding this comment

rwe commented Jul 31, 2017 • edited

asaf-romano commented Aug 4, 2017

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Dec 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron commented Dec 21, 2017

leebyron commented Dec 21, 2017

IvanGoncharov commented Dec 21, 2017

IvanGoncharov commented Feb 5, 2018 • edited

IvanGoncharov commented Sep 20, 2018

IvanGoncharov commented Oct 14, 2018

leebyron commented Jun 16, 2017 •

edited

leebyron Jun 16, 2017 •

edited

rwe commented Jul 31, 2017 •

edited

IvanGoncharov Dec 21, 2017 •

edited

IvanGoncharov commented Feb 5, 2018 •

edited