Skip to content
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: Define custom scalars in terms of built-in scalars. #914

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/language/__tests__/schema-kitchen-sink.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ scalar CustomScalar

scalar AnnotatedScalar @onScalar

scalar StringEncodedCustomScalar as String

extend scalar CustomScalar @onScalar

enum Site {
Expand Down
1 change: 1 addition & 0 deletions src/language/__tests__/schema-parser-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,7 @@ type Hello {
kind: 'ScalarTypeDefinition',
name: nameNode('Hello', { start: 7, end: 12 }),
description: undefined,
type: undefined,
directives: [],
loc: { start: 0, end: 12 },
},
Expand Down
2 changes: 2 additions & 0 deletions src/language/__tests__/schema-printer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ describe('Printer: SDL document', () => {

scalar AnnotatedScalar @onScalar

scalar StringEncodedCustomScalar as String

extend scalar CustomScalar @onScalar

enum Site {
Expand Down
1 change: 1 addition & 0 deletions src/language/ast.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ export type ScalarTypeDefinitionNode = {
+loc?: Location,
+description?: StringValueNode,
+name: NameNode,
+type?: NamedTypeNode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Fully agree. How about serializableAs?

Copy link
Member

Choose a reason for hiding this comment

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

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

+directives?: $ReadOnlyArray<DirectiveNode>,
};

Expand Down
26 changes: 24 additions & 2 deletions src/language/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -864,18 +864,26 @@ function parseOperationTypeDefinition(
}

/**
* ScalarTypeDefinition : Description? scalar Name Directives[Const]?
* ScalarTypeDefinition :
* - Description? scalar Name ScalarOfType? Directives[Const]?
*
* ScalarOfType : as NamedType
*/
function parseScalarTypeDefinition(lexer: Lexer<*>): ScalarTypeDefinitionNode {
const start = lexer.token;
const description = parseDescription(lexer);
expectKeyword(lexer, 'scalar');
const name = parseName(lexer);
let type;
if (skipKeyword(lexer, 'as')) {
type = parseNamedType(lexer);
}
const directives = parseDirectives(lexer, true);
return {
kind: Kind.SCALAR_TYPE_DEFINITION,
description,
name,
type,
directives,
loc: loc(lexer, start),
};
Expand Down Expand Up @@ -1512,10 +1520,24 @@ function expect(lexer: Lexer<*>, kind: TokenKindEnum): Token {
}

/**
* If the next token is a keyword with the given value, return that token after
* If the next token is a keyword with the given value, return true after
* advancing the lexer. Otherwise, do not change the parser state and return
* false.
*/
function skipKeyword(lexer: Lexer<*>, value: string): boolean {
const token = lexer.token;
const match = token.kind === TokenKind.NAME && token.value === value;
if (match) {
lexer.advance();
}
return match;
}

/**
* If the next token is a keyword with the given value, return that token after
* advancing the lexer. Otherwise, do not change the parser state and throw
* an error.
*/
function expectKeyword(lexer: Lexer<*>, value: string): Token {
const token = lexer.token;
if (token.kind === TokenKind.NAME && token.value === value) {
Expand Down
4 changes: 2 additions & 2 deletions src/language/printer.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ const printDocASTReducer = {

OperationTypeDefinition: ({ operation, type }) => operation + ': ' + type,

ScalarTypeDefinition: addDescription(({ name, directives }) =>
join(['scalar', name, join(directives, ' ')], ' '),
ScalarTypeDefinition: addDescription(({ name, type, directives }) =>
join(['scalar', name, wrap('as ', type), join(directives, ' ')], ' '),
),

ObjectTypeDefinition: addDescription(
Expand Down
2 changes: 1 addition & 1 deletion src/language/visitor.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export const QueryDocumentKeys = {
SchemaDefinition: ['directives', 'operationTypes'],
OperationTypeDefinition: ['type'],

ScalarTypeDefinition: ['description', 'name', 'directives'],
ScalarTypeDefinition: ['description', 'name', 'type', 'directives'],
ObjectTypeDefinition: [
'description',
'name',
Expand Down
4 changes: 3 additions & 1 deletion src/type/__tests__/introspection-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1355,7 +1355,9 @@ describe('Introspection', () => {
'An enum describing what kind of type a given `__Type` is.',
enumValues: [
{
description: 'Indicates this type is a scalar.',
description:
'Indicates this type is a scalar. ' +
'`ofType` may represent how this scalar is serialized.',
name: 'SCALAR',
},
{
Expand Down
14 changes: 12 additions & 2 deletions src/type/definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -538,17 +538,26 @@ export class GraphQLScalarType {
serialize: GraphQLScalarSerializer<*>;
parseValue: GraphQLScalarValueParser<*>;
parseLiteral: GraphQLScalarLiteralParser<*>;
ofType: ?GraphQLScalarType;
astNode: ?ScalarTypeDefinitionNode;
extensionASTNodes: ?$ReadOnlyArray<ScalarTypeExtensionNode>;

constructor(config: GraphQLScalarTypeConfig<*, *>): void {
this.name = config.name;
this.description = config.description;
this.serialize = config.serialize;
this.parseValue = config.parseValue || (value => value);
this.parseLiteral = config.parseLiteral || valueFromASTUntyped;
this.ofType = config.ofType || null;
this.parseValue =
config.parseValue ||
(this.ofType && this.ofType.parseValue) ||
(value => value);
this.parseLiteral =
config.parseLiteral ||
(this.ofType && this.ofType.parseLiteral) ||
valueFromASTUntyped;
this.astNode = config.astNode;
this.extensionASTNodes = config.extensionASTNodes;

invariant(typeof config.name === 'string', 'Must provide name.');
invariant(
typeof config.serialize === 'function',
Expand Down Expand Up @@ -591,6 +600,7 @@ export type GraphQLScalarTypeConfig<TInternal, TExternal> = {|
parseValue?: GraphQLScalarValueParser<TInternal>,
// Parses an externally provided literal value to use as an input.
parseLiteral?: GraphQLScalarLiteralParser<TInternal>,
ofType?: ?GraphQLScalarType,
astNode?: ?ScalarTypeDefinitionNode,
extensionASTNodes?: ?$ReadOnlyArray<ScalarTypeExtensionNode>,
|};
Expand Down
8 changes: 5 additions & 3 deletions src/type/introspection.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,8 @@ export const __Type = new GraphQLObjectType({
'The fundamental unit of any GraphQL Schema is the type. There are ' +
'many kinds of types in GraphQL as represented by the `__TypeKind` enum.' +
'\n\nDepending on the kind of a type, certain fields describe ' +
'information about that type. Scalar types provide no information ' +
'beyond a name and description, while Enum types provide their values. ' +
'information about that type. Scalar types provide a name, description ' +
'and how they serialize, while Enum types provide their possible values. ' +
'Object and Interface types provide the fields they describe. Abstract ' +
'types, Union and Interface, provide the Object types possible ' +
'at runtime. List and NonNull types compose other types.',
Expand Down Expand Up @@ -399,7 +399,9 @@ export const __TypeKind = new GraphQLEnumType({
values: {
SCALAR: {
value: TypeKind.SCALAR,
description: 'Indicates this type is a scalar.',
description:
'Indicates this type is a scalar. ' +
'`ofType` may represent how this scalar is serialized.',
},
OBJECT: {
value: TypeKind.OBJECT,
Expand Down
22 changes: 21 additions & 1 deletion src/type/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/

import {
isScalarType,
isObjectType,
isInterfaceType,
isUnionType,
Expand All @@ -19,6 +20,7 @@ import {
isRequiredArgument,
} from './definition';
import type {
GraphQLScalarType,
GraphQLObjectType,
GraphQLInterfaceType,
GraphQLUnionType,
Expand All @@ -28,6 +30,7 @@ import type {
import { isDirective } from './directives';
import type { GraphQLDirective } from './directives';
import { isIntrospectionType } from './introspection';
import { isSpecifiedScalarType } from './scalars';
import { isSchema } from './schema';
import type { GraphQLSchema } from './schema';
import inspect from '../jsutils/inspect';
Expand Down Expand Up @@ -244,7 +247,10 @@ function validateTypes(context: SchemaValidationContext): void {
validateName(context, type);
}

if (isObjectType(type)) {
if (isScalarType(type)) {
// Ensure Scalars can serialize as expected.
validateScalarSerialization(context, type);
} else if (isObjectType(type)) {
// Ensure fields are valid
validateFields(context, type);

Expand All @@ -266,6 +272,20 @@ function validateTypes(context: SchemaValidationContext): void {
}
}

function validateScalarSerialization(
context: SchemaValidationContext,
scalarType: GraphQLScalarType,
): void {
if (scalarType.ofType && !isSpecifiedScalarType(scalarType.ofType)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@IvanGoncharov IvanGoncharov Dec 21, 2017

Choose a reason for hiding this comment

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

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.

context.reportError(
`Scalar type ${scalarType.name} may only be described in terms of a ` +
`spec-defined scalar type. However ${String(scalarType.ofType)} is ` +
'not a built-in scalar type.',
scalarType.astNode && scalarType.astNode.type,
);
}
}

function validateFields(
context: SchemaValidationContext,
type: GraphQLObjectType | GraphQLInterfaceType,
Expand Down
35 changes: 25 additions & 10 deletions src/utilities/__tests__/schemaPrinter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,17 @@ describe('Type System Printer', () => {
});

it('Custom Scalar', () => {
const EvenType = new GraphQLScalarType({
name: 'Even',
ofType: GraphQLInt,
serialize(value) {
return value % 2 === 1 ? value : null;
},
});

const OddType = new GraphQLScalarType({
name: 'Odd',
// No ofType in this test case.
serialize(value) {
return value % 2 === 1 ? value : null;
},
Expand All @@ -471,16 +480,20 @@ describe('Type System Printer', () => {
const Query = new GraphQLObjectType({
name: 'Query',
fields: {
even: { type: EvenType },
odd: { type: OddType },
},
});

const Schema = new GraphQLSchema({ query: Query });
const output = printForTest(Schema);
expect(output).to.equal(dedent`
scalar Even as Int

scalar Odd

type Query {
even: Even
odd: Odd
}
`);
Expand Down Expand Up @@ -785,10 +798,10 @@ describe('Type System Printer', () => {
types in GraphQL as represented by the \`__TypeKind\` enum.

Depending on the kind of a type, certain fields describe information about that
type. Scalar types provide no information beyond a name and description, while
Enum types provide their values. Object and Interface types provide the fields
they describe. Abstract types, Union and Interface, provide the Object types
possible at runtime. List and NonNull types compose other types.
type. Scalar types provide a name, description and how they serialize, while
Enum types provide their possible values. Object and Interface types provide the
fields they describe. Abstract types, Union and Interface, provide the Object
types possible at runtime. List and NonNull types compose other types.
"""
type __Type {
kind: __TypeKind!
Expand All @@ -804,7 +817,9 @@ describe('Type System Printer', () => {

"""An enum describing what kind of type a given \`__Type\` is."""
enum __TypeKind {
"""Indicates this type is a scalar."""
"""
Indicates this type is a scalar. \`ofType\` may represent how this scalar is serialized.
"""
SCALAR

"""
Expand Down Expand Up @@ -1001,10 +1016,10 @@ describe('Type System Printer', () => {
# types in GraphQL as represented by the \`__TypeKind\` enum.
#
# Depending on the kind of a type, certain fields describe information about that
# type. Scalar types provide no information beyond a name and description, while
# Enum types provide their values. Object and Interface types provide the fields
# they describe. Abstract types, Union and Interface, provide the Object types
# possible at runtime. List and NonNull types compose other types.
# type. Scalar types provide a name, description and how they serialize, while
# Enum types provide their possible values. Object and Interface types provide the
# fields they describe. Abstract types, Union and Interface, provide the Object
# types possible at runtime. List and NonNull types compose other types.
type __Type {
kind: __TypeKind!
name: String
Expand All @@ -1019,7 +1034,7 @@ describe('Type System Printer', () => {

# An enum describing what kind of type a given \`__Type\` is.
enum __TypeKind {
# Indicates this type is a scalar.
# Indicates this type is a scalar. \`ofType\` may represent how this scalar is serialized.
SCALAR

# Indicates this type is an object. \`fields\` and \`interfaces\` are valid fields.
Expand Down
4 changes: 4 additions & 0 deletions src/utilities/buildASTSchema.js
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,10 @@ export class ASTDefinitionBuilder {
return new GraphQLScalarType({
name: def.name.value,
description: getDescription(def, this._options),
// Note: While this could make assertions to get the correctly typed
// values below, that would throw immediately while type system
// validation with validateSchema() will produce more actionable results.
ofType: def.type && (this.buildType(def.type): any),
astNode: def,
serialize: value => value,
});
Expand Down
4 changes: 4 additions & 0 deletions src/utilities/buildClientSchema.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,13 @@ export function buildClientSchema(
function buildScalarDef(
scalarIntrospection: IntrospectionScalarType,
): GraphQLScalarType {
const ofType = scalarIntrospection.ofType
? (getType(scalarIntrospection.ofType): any)
: undefined;
return new GraphQLScalarType({
name: scalarIntrospection.name,
description: scalarIntrospection.description,
ofType,
serialize: value => value,
});
}
Expand Down
12 changes: 12 additions & 0 deletions src/utilities/findBreakingChanges.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,18 @@ export function findTypesThatChangedKind(
`${typeName} changed from ` +
`${typeKindName(oldType)} to ${typeKindName(newType)}.`,
});
} else if (isScalarType(oldType) && isScalarType(newType)) {
const oldOfType = oldType.ofType;
const newOfType = newType.ofType;
if (oldOfType && newOfType && oldOfType !== newOfType) {
breakingChanges.push({
type: BreakingChangeType.TYPE_CHANGED_KIND,
description:
`${typeName} changed from ` +
`${typeKindName(oldType)} serialized as ${oldOfType.name} ` +
`to ${typeKindName(newType)} serialized as ${newOfType.name}.`,
});
}
}
}
return breakingChanges;
Expand Down
1 change: 1 addition & 0 deletions src/utilities/introspectionQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ export type IntrospectionScalarType = {
+kind: 'SCALAR',
+name: string,
+description?: ?string,
+ofType?: ?IntrospectionNamedTypeRef<IntrospectionScalarType>,
};

export type IntrospectionObjectType = {
Expand Down
3 changes: 2 additions & 1 deletion src/utilities/schemaPrinter.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ export function printType(type: GraphQLNamedType, options?: Options): string {
}

function printScalar(type: GraphQLScalarType, options): string {
return printDescription(options, type) + `scalar ${type.name}`;
const ofType = type.ofType ? ` as ${type.ofType.name}` : '';
return printDescription(options, type) + `scalar ${type.name}${ofType}`;
}

function printObject(type: GraphQLObjectType, options): string {
Expand Down