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

Clarify note about unused custom scalars #648

Closed
sungam3r opened this issue Nov 17, 2019 · 10 comments
Closed

Clarify note about unused custom scalars #648

sungam3r opened this issue Nov 17, 2019 · 10 comments

Comments

@sungam3r
Copy link
Contributor

Relates to #222

Regarding this comment.

Do I understand correctly that custom scalars (not built-in, for example, DateTime, Seconds, Uri) defined by this or that GraphQL server implementation should not be present in the SDL if they are not used there? If this is so, then it seems to me that it is worth explicitly declaring this in the specification. Now there it is approved only for built-in scalars. I assume that any unused elements of the schema should not be present in it, not just scalars. In fact, these are simply non-things and they should be discarded. Just wanted to clarify this issue.

My example is that now the graphql-dotnet always generates SDL with all the custom scalars it has inside internal registry:

scalar BigInt

scalar Byte

scalar Date

scalar DateTime

scalar DateTimeOffset

scalar Decimal

scalar Guid

scalar Long

scalar Milliseconds

scalar SByte

scalar Seconds

scalar Short

scalar UInt

scalar ULong

scalar UShort

scalar Uri

It seems to me that this is wrong.

@michaelstaib
Copy link
Member

In hot chocolate for .NET we are only including types that are used in order to keep the schema as small as possible. However, we allow that you can have loose ends in a special development mode.

@sungam3r
Copy link
Contributor Author

@benjie , @IvanGoncharov What is your opinion?

@benjie
Copy link
Member

benjie commented Nov 22, 2019

I've not looked at the issue in depth, but I don't think the schema should contain types that are not usable, scalar or otherwise.

@benjie
Copy link
Member

benjie commented Nov 22, 2019

To back up my previous statement with spec text (emphasis mine):

A server may omit any of the built‐in scalars from its schema, for example if a schema does not refer to a floating‐point number, then it may omit the Float type.

and

When representing a GraphQL schema using the type system definition language, the built‐in scalar types should be omitted for brevity.

So it seems the spec is flexible on this matter when it comes to the built in scalars.

The spec also states:

A schema is defined in terms of the types and directives it supports

I'd argue that a schema that does not use a custom scalar type in input types, output types or directives doesn't really "support" that scalar - it cannot be used anywhere.

@sungam3r
Copy link
Contributor Author

My question is whether this should be explicitly stated in the specification? We are now talking about our interpretations of the specification. We can interpret in different ways. I would like to avoid that. Let it be better clearly indicated whether the schema can include unused elements (scalars, types - any, it has already become clear that the question went beyond the scope of scalars.). For some reason, a schema with unused elements can be built. Is it valid?

Is such a schema valid?

type Query {
  field1: String!
  field2: MyType1
}

type MyType1
{
  intField: Int!
}

type MyType347
{
  boolField: Bool!
}

Suppose I programmatically build a schema and make a mistake as a result of which such an SDL is obtained.

P.S. I myself understand that the schema should not contain unused elements. But these are my speculations, and not what the specification requires.

@sungam3r
Copy link
Contributor Author

And here I am smoothly approaching the issue of schema validation. The specification describes the rules for validating incoming documents (queries/requests). But clearly does not speak of any set of rules for validation of SDLs. We can build syntactically correct SDL, which nonetheless will contain unused elements.

@benjie
Copy link
Member

benjie commented Nov 24, 2019

Yeah, I’m agreeing that the spec seems to already imply this, but stating it explicitly seems sensible. Including types that aren’t used will just bloat the introspection results unnecessarily.

@sungam3r
Copy link
Contributor Author

It looks like the spec has become clearer with respect to embedded scalars - #597 :

When returning the set of types from the __Schema introspection type, all
referenced built-in scalars must be included. If a built-in scalar type is not
referenced anywhere in a schema (there is no field, argument, or input field of
that type) then it must not be included.

+

When representing a GraphQL schema using the type system definition language,
all built-in scalars must be omitted for brevity.

@sungam3r
Copy link
Contributor Author

@benjie Will we consider that this issue can be closed? From my point of view clarifications appeared in #597 fully answer my initial question about scalars. Nevertheless, there remains some understatement about unused types in general. I mean this part of spec which you mentioned:

A schema is defined in terms of the types and directives it supports

I'd argue that a schema that does not use a custom scalar type in input types, output types or directives doesn't really "support" that scalar - it cannot be used anywhere.

Is it worth revealing the meaning of supports in this part of the spec?

@benjie
Copy link
Member

benjie commented Jan 24, 2020

I think it's probably fine to leave it as-is - +1 for closing this issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants