graphql.github.io icon indicating copy to clipboard operation
graphql.github.io copied to clipboard

Add Schema Coordinates functions documentation

Open magicmark opened this issue 5 years ago • 8 comments

Addresses https://github.com/graphql/graphql-wg/issues/581

Description

Schema Coordinates are being proposed as a way to uniquely identify a specific type, field, argument, enum value, or directive defined in a GraphQL Schema.

In order to move to Stage 2, we need to add functions for parsing/printing schema coordinates to graphql-js.

This PR sketches out the interface for the functions I'll be working on. I'm making a PR now to share this early on, before the real implementation so we can hash out the naming/arguments etc.

cc @IvanGoncharov

magicmark avatar Jan 10 '21 22:01 magicmark

One thing that's a bit of an open question for me is the "Schema Coordinate AST":

Lee: regarding reference implementation changes, we should add at least parsing functions to allow parsing into an AST and printing an AST back out again. That would give people the tools they need to use this; and I think that's needed for RFC2.

This seems like a new thing we'll need to come up with - which makes sense. Ideally i'd like to be able to turn each branch in the spec (https://github.com/graphql/graphql-spec/pull/794/files#diff-30a69c5a5eded8e1aea52e53dad1181e6ec8f549ca2c50570b035153e2de1c43R257) into possible ASTs and just follow that.

However, we need to deal with schema coordinates in an abstract way. At parse time, for Foo.baz, we don't know if Foo is an interface, object type or enum etc. So we need something like:

{
  "kind": "SchemaCoordinate",
  "definitions": [
    {
      "kind": "InterfaceOrObjectOrEnum",
      "value": "Foo",
    }
    ...

Which is p ugly, but it seems like we need to invent something here. (The spec just uses the abstract 'Name'.) Thoughts?

magicmark avatar Jan 10 '21 22:01 magicmark

For Name . Name ( Name : ):

  • I would use TypeName for the AST node represented by the first Name in Name . Name; you want it to remain valid even as we extend GraphQL with more types (e.g. tagged type).
  • I would use FieldName for the AST node for the second Name (though it may relate to a field, input field, member field, enum value). Another option might be ChildName, but I think I'd use field.
  • I would use ArgumentName for the AST node for the third name. I'd also use this for the directive argument name.

benjie avatar Jan 11 '21 11:01 benjie

However, we need to deal with schema coordinates in an abstract way. At parse time, for Foo.baz, we don't know if Foo is an interface, object type or enum etc.

This was my point during WG. We don't need to distinguish between objects and interfaces since they have the exact same form of coordinates. But I think it worth to use something like :: as separator for enum values. That way we will have two distinct AST chains:

  • Type => Field => Arg
  • Type => EnumValue

IvanGoncharov avatar Jan 11 '21 11:01 IvanGoncharov

Name . Name I think covers anything field-like (object type fields, input object input fields, interface fields, tagged type member fields, etc), putting enum values under the same syntax does feel a little weird.

benjie avatar Jan 11 '21 12:01 benjie

Coming back to this! Thanks for the detailed thoughts y'all :)

  • But I think it worth to use something like :: as separator for enum values.
  • putting enum values under the same syntax does feel a little weird.

Sounds like we have two votes for changing direction here a little bit - and using something like Foo::BAR syntax for enums.

fwiw I'm personally not bothered by the fact . is overloaded, the fact that enum values are usually uppercased makes it clear enough for me, and I'd be happy adding complexity to an internal function for the sake of readability everywhere else.

Let's use :: for now, and see what it looks like! I'm totally ok either way here (and certainly happy to make this implementation easier :P). Keen to hear what other folks think.

@benjie I assume this will affect operation expressions too. Will using :: make the AST parsing for that harder? (Since I see : is already a separator used in expressions) We might be able to save some headaches here by not stomping on ourselves syntax-wise. Thoughts?

~Aaaaalso, even with a separate separator for enums, for Foo.bar, we still don't know if Foo is an object, input object or interface. e.g. we'd still need a ObjectOrInterface node, which we can't break down any further.~

~That said, since nodeFromSchemaCoordinate takes in the schema as well, we inspect the schema to break this down further. So all good.~ (sorry just thinking out loud here)

magicmark avatar Mar 07 '21 10:03 magicmark

ok here's an early sketch of what I have so far for the nodes: https://gist.github.com/magicmark/16bd0d7fbb987fc7f75b536993bdbaa5

(warning: i am dumb frontend developer. No real idea what i'm doing here other than snooping on prior art within graphql-js and other languages on astexplorer.net, please bear with me :D)

magicmark avatar Mar 07 '21 11:03 magicmark

I can't think of an issue with operation expressions/enums explicitly, but a field and an enum value are sufficiently distinct concepts that I think using a separate separator (e.g. :: as proposed) would make sense and make future expansion easier.

benjie avatar Mar 15 '21 13:03 benjie