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

Add Schema Coordinates functions documentation #998

Open
wants to merge 1 commit into
base: source
Choose a base branch
from

Conversation

magicmark
Copy link

Addresses graphql/graphql-wg#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
Copy link
Author

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?

@benjie
Copy link
Member

benjie commented Jan 11, 2021

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.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jan 11, 2021

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

@benjie
Copy link
Member

benjie commented Jan 11, 2021

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.

@magicmark
Copy link
Author

magicmark commented Mar 7, 2021

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
Copy link
Author

magicmark commented Mar 7, 2021

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)

@benjie
Copy link
Member

benjie commented Mar 15, 2021

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.

@saihaj
Copy link
Member

saihaj commented Dec 17, 2024

I think this should now live under the new GraphQL JS website. cc @graphql/graphql-js-reviewers

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

Successfully merging this pull request may close these issues.

4 participants