GAP-4 Add Field Extensions Spec

[egoodwinx]

Copied from graphql/graphql-spec#1196 and graphql/graphql-spec#1162

[magicmark]

# This is not valid GraphQL

So is the idea to compile this out of the SDL before passing to a GraphQL server?

[egoodwinx]

So is the idea to compile this out of the SDL before passing to a GraphQL server?

Yes

[magicmark]

I think this would benefit then from being layered on top of the soon-to-be-proposed language in the spec to clarify the difference between "human written"(?) schemas and "runtime" schemas. Not required for this to merge, but worth following along!

@benjie wdyt about a dedicated 2xxx range for "language and syntax extensions"?

side note, not directly relevant to this proposal but:

• since this is valid syntax folks don't need to worry about existing tooling (syntax highlighters, ast parsing functions and such) breaking on schema that uses this - I think it's only invalid at runtime when validation rules get applied.
• I wonder what should happen if a GAP proposes new syntax - and at that point you're on your own, there's no guarantee that any other tooling would work. Obviously this isn't a problem to worry about for now, and could be solvable in future I imagine with plugins (similar to babel) and whatnot.

[mjmahone]

Ugh as a GAP proposal figuring out the difference between a field extension and an original field definition inside an extension is tough, because I think we must have some kind of syntax, or at least a special directive.

I ran into a bunch of bugs before getting an internal representation where fields can be pure extensions, and I still have unresolvable bugs in going from SDL => SchemaSet (then do work on that set) => SDL. It's very frustrating.

[mjmahone]

I actually think it would be cleaner to do:

Suggested change

	extend type Query {
	id: ID @deprecated(reason: "Use globalId instead")
	}
	extend type Query {
	extend id: ID @deprecated(reason: "Use globalId instead")
	}

Basically you're being explicit that this field DOES already exist but you're adding more to it.

I have this exact situation and have an internal representation that we actually see that I can't do a round trip into the SDL and back into SchemaSet in https://github.com/facebook/relay/blob/main/compiler/crates/schema-set/src/set_exclude.rs#L413

[egoodwinx]

I'm good with this syntax.

[benjie]

For field extensions:

• Replacing the response type is not an extension, so the type should be excluded
• Replacing an argument is not an extension, so the existing arguments should be excluded
• Maybe you want to add an argument
• Maybe you want to add a directive
• Maybe you want to add a description (?)

So I'd propose:

extend type Query {
  # Add a directive to `Query.id`
  extend id @deprecated(reason: "Use globalId instead")

  # Add an argument to `Query.findUsers`
  extend findUsers(includeArchived: Boolean! = false)
  
  # Add a description to `Query.currentUser`
  """The person who is currently logged into the API, if any"""
  extend currentUser
}

[egoodwinx]

I actually talked to @martinbonnin and since the only implementation is currently in Kotlin we would prefer to keep it as the single extends for now. I am going to work on the graphql-js implementation next.

[martinbonnin]

Yup, we have implemented the very simple version of it in Apollo Kotlin:

      type Query {
        random: Int
      }      
      extend type Query {
        random: Int @deprecated
      }

I agree we can probably do better but that's not something we're going to pursue in the short term. The main use case we wanted to unlock was adding directive to existing field definitions. If we could point users at this GAP, it'd be useful. If not, it's not big deal either.

[martinbonnin]

@magicmark @egoodwinx can we get that one merged in? I recognize this is probably not the perfect endgame solution but it's serving us well and we have this implemented in Apollo Kotlin. Would be cooil to be able to have a GAP number attached to it.

[magicmark]

@martinbonnin good to merge pending some fiddly process things (apologies):

• metadata.yml needs fixing up
• this is GAP-4, not GAP-2 (this pr was opened before the great renumbering)

[benjie]

@egoodwinx I took the liberty of rebasing your last two commits so 32e22ef now includes deletion of GAP-2 (7f7e887 only added the new files, didn't remove the old) so it shows up in history as a rename now.

[egoodwinx]

@egoodwinx I took the liberty of rebasing your last two commits so 32e22ef now includes deletion of GAP-2 (7f7e887 only added the new files, didn't remove the old) so it shows up in history as a rename now.

Whoops - thanks! I ran the prettier command and it reformatted all the files for some reason so I must have missed it.