-
Notifications
You must be signed in to change notification settings - Fork 2k
RFC: Define custom scalars in terms of built-in scalars. #914
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
Changes from all commits
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 |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
*/ | ||
|
||
import { | ||
isScalarType, | ||
isObjectType, | ||
isInterfaceType, | ||
isUnionType, | ||
|
@@ -19,6 +20,7 @@ import { | |
isRequiredArgument, | ||
} from './definition'; | ||
import type { | ||
GraphQLScalarType, | ||
GraphQLObjectType, | ||
GraphQLInterfaceType, | ||
GraphQLUnionType, | ||
|
@@ -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'; | ||
|
@@ -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); | ||
|
||
|
@@ -266,6 +272,20 @@ function validateTypes(context: SchemaValidationContext): void { | |
} | ||
} | ||
|
||
function validateScalarSerialization( | ||
context: SchemaValidationContext, | ||
scalarType: GraphQLScalarType, | ||
): void { | ||
if (scalarType.ofType && !isSpecifiedScalarType(scalarType.ofType)) { | ||
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 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 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 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(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 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.
We can allow specifying scalar serializable as any other scalar without scalar BigInt
scalar Counter as BigInt # Valid
scalar UserCounter as Counter # Invalid But with current terminology and SDL syntax, it would be very confusing 😞 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, | ||
|
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.
I think this key should be more descriptive. A type having a field called "type" is confusing
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.
Fully agree. How about
serializableAs
?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.
@leebyron Given current syntax
scalar Url as String
.How about
asType
?