graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

Allow empty object type _extensions_ in schema #1106

Open ychescale9 opened 4 months ago

ychescale9 commented 4 months ago

I have been told by @martinbonnin that the following isn't supposed to work according to the spec:

extend type Query {

}

extend type Mutation {

}

However being able to have empty type definitions is useful sometimes where a local "stub" schema needs to be changed between empty and non-empty frequently as part of the dev workflow.

Specifically the use case for us is that we have a local stub.graphqls schema alongside the remote schema we download from the backend. Our devs sometimes add types to the stubs.graphqls while waiting for the changes to be deployed to the real schema (we try to avoid manually tempering the real schema). So about half of the time the content of the extend type Query is empty.

A similar use case can be found here.

benjie commented 4 months ago

Duplicate of https://github.com/graphql/graphql-spec/issues/568 - please re-post any new interesting feedback to that thread.

Also related: https://github.com/graphql/graphql-spec/issues/236

martinbonnin commented 4 months ago

Thanks for opening this @ychescale9!

@benjie I'd argue this issue is slightly different from #568: #568 is about empty types and this issue is about empty exensions.

568 is probably a more fundamental change to the GraphQL type system. I see this issue as mainly about tooling. It doesn't change how the types are represented. Unions are still a thing and objects are required to have one or more fields. Just for convenience, we would allow extensions to be empty. That is a much smaller lift than #568.

Can we keep it open with Allow empty object and interface extensions as a title?

benjie commented 4 months ago

We already allow extend type Query to exist. Can you not simply omit the {} when there's nothing in the braces?

I'm pretty sure this parses, though I haven't checked:

type Query {
  meaningOfLife: Int
}
extend type Query
martinbonnin commented 4 months ago

Agreed, extend type Query should reasonably work.

I think the intent here is facilitating copy/pasting full lines. I see this a bit as trailing comma quality of life improvement. Doesn't really change the language but makes it easier to work with the documents on a daily basis.

It also minimizes the diff if you were to commit those files, etc... It's not essential but I would support the change unless there are strong reasons not to do so that I'm not thinking about today.

ychescale9 commented 4 months ago

@martinbonnin FYI with Apollo Kotlin just having extend type Query throws: Unexpected token: 'name: extend'.

martinbonnin commented 4 months ago

Yikes, let me double check. Just to make sure: is there anything before that extend type Query? If it's not expecting extend, it probably means the parser is left hanging on the previous expression.

ychescale9 commented 4 months ago

Yikes, let me double check. Just to make sure: is there anything before that extend type Query? If it's not expecting extend, it probably means the parser is left hanging on the previous expression.

Just tested again: this is the entire document:

extend type Query

Result:

path/to/stub.graphqls: (2, 1): Unexpected token: 'EOF'
  ----------------------------------------------------
  [1]:extend type Query
  [2]:

  ----------------------------------------------------
martinbonnin commented 4 months ago

Turns out extend type Query not parsing is working as expected.

See https://spec.graphql.org/draft/#ObjectTypeExtension, the spec mandates that the extension adds at least a directive, a field or an interface.

benjie commented 4 months ago

That makes sense; extend type Query on it's own would be as meaningless (no-op) as extend type Query {}. Trailing commas are an interesting analogy; all commas are ignored in GraphQL - type,Query,{,a:,Int!,,},, is valid I think - but braces are significant so we can't apply the same kind of logic alas. The question really is: is there sufficient utility in this suggestion to justify making every GraphQL implementation support it?

martinbonnin commented 4 months ago

is there sufficient utility in this suggestion to justify making every GraphQL implementation support it?

That's the billion dollar question. My initial thoughts are that parsers are written by few people (dozens?) but used by many more (thousands? millions?). So the price to pay for the maintainers is amortized on a lot of potential users.

Now it all depends how many users would actually benefit from this feature... #tradeoffs