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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Schema Coordinates #746

Merged
merged 16 commits into from Jan 7, 2021
Merged

Conversation

magicmark
Copy link
Contributor

@magicmark magicmark commented Jul 4, 2020

Here's a stab at the RFC for #735 - field coordinates

馃憠 Read in fancy format: https://github.com/magicmark/graphql-spec/blob/add_field_coordinates/rfcs/SchemaCoordinates.md
馃憠 See the proposed spec: #794

What does this RFC propose?

As a quick reminder, given the following schema:

type Person {
  name: String
}

type Business {
  name: String
  owner: String
}
  • Person.name uniquely identifies the "name" field on the "Person" type
  • Business.name uniquely identifies the "name" field on the "Business" type
  • Business.owner uniquely identifies the "owner" field on the "Business" type

3rd party tooling uses this notation as a convention - however there is no official standardization (yet!) on what these are called, or how they're defined. We'll henceforth refer to these as "schema coordinates".


Looking forward to working with y'all to figure out if this seems worth advancing / hearing feedback / figuring out next steps.

Thanks!

@magicmark magicmark force-pushed the add_field_coordinates branch 2 times, most recently from 858a105 to 72b0155 Compare July 4, 2020 08:12
rfcs/FieldCoordinates.md Outdated Show resolved Hide resolved
rfcs/FieldCoordinates.md Outdated Show resolved Hide resolved
rfcs/FieldCoordinates.md Outdated Show resolved Hide resolved
rfcs/FieldCoordinates.md Outdated Show resolved Hide resolved
rfcs/FieldCoordinates.md Outdated Show resolved Hide resolved
rfcs/FieldCoordinates.md Outdated Show resolved Hide resolved
@magicmark
Copy link
Contributor Author

Thanks @sungam3r for the review! Sorry for the typos, have installed a spellcheck in vscode now :)

@magicmark
Copy link
Contributor Author

@IvanGoncharov Hey! This is my rodeo with the GQL RFC process - are there any more steps I can take for now? What should I be expecting at this stage? Thanks!

magicmark added a commit to magicmark/graphql-wg that referenced this pull request Aug 1, 2020
馃憢 hi!

Would love to get some discussion going on graphql/graphql-spec#735 / graphql/graphql-spec#746 so I can figure out what (or rather "if") what the next steps might be

thanks!
leebyron added a commit to graphql/graphql-wg that referenced this pull request Aug 5, 2020
馃憢 hi!

Would love to get some discussion going on graphql/graphql-spec#735 / graphql/graphql-spec#746 so I can figure out what (or rather "if") what the next steps might be

thanks!

Co-authored-by: Lee Byron <lee@leebyron.com>
@fotoetienne
Copy link
Contributor

Would InterfaceName.fieldName also be a valid field coordinate, or would it need to be the concrete implementing type?

@IvanGoncharov IvanGoncharov added 馃挱 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) 馃挕 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) and removed 馃挱 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels Aug 19, 2020
@magicmark magicmark changed the title RFC: Field coordinates RFC: Schema Coordinates Sep 17, 2020
rfcs/SchemaCoordinates.md Outdated Show resolved Hide resolved
rfcs/SchemaCoordinates.md Outdated Show resolved Hide resolved
rfcs/SchemaCoordinates.md Outdated Show resolved Hide resolved
- `Business.owner` uniquely identifies the "owner" field on the "Business" type
- `Query.searchBusinesses` uniquely identifies the "searchBusinesses" field on
the "Query" type
- `Query.searchBusinesses(name:)` uniquely identifies the "name" argument on the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not keen on this syntax for arguments. Query.searchBusinesses.name feels a bit more natural to me, and is less likely to be mistaken for an actual query fragment or something. But I don't have a compelling argument here, it's mostly just personal preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we stick with this syntax we can extend it in future to reference a position in a GraphQL document; e.g.

Query.businesses:searchBusinesses(name:).owner:personByOwnerId.email

might refer to:

{
  businesses: searchBusinesses(name: "Automotive") {
    id
    name
    owner: personByOwnerId {
      id
      name
      email # <<< HERE
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's a poor example (probably you'd just say businesses.owner.email); but you could write something like the above expression and have it automatically expand to a similar GraphQL document, similar to how you can use CSS shortcuts to expand to DOM structure using emmet or similar.

Copy link
Contributor Author

@magicmark magicmark Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eapache Happy to go either way! I'm pretty 50/50 on Foo.bar.baz vs Foo.bar(baz:). How do we decide here? Toss a coin? Dance-off battle?

@benjie On the topic of something like Query.businesses:searchBusinesses(name:).owner:personByOwnerId.email, just to clarify:

  1. Schema coordinates are supposed to "uniquely identify a type". Given our use cases, it's ideal if there's no normalization that has to be done to get to the "simplest" schema coordinate to represent the same thing.
  2. Tracing the path to a member of a query/document is kinda already a thing with field paths (e.g. "path": [ "hero", "heroFriends", 1, "name" ]).

Perhaps it's possible one day we unify this syntax and expand schema coordinates to support nesting, but I'll gingerly suggest that this be out of scope for this particular RFC? (Worth bearing in mind for a future extension of this syntax tho, along with selecting multiple arguments)

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dance-off battle?

I'm in 馃槀

Seriously though, I think it's probably something worth chatting about at the IRL working group meetings. There might be an obvious consensus there, and if not then we'll probably want to do a slightly more formal pros/cons writeup to justify making a final call (basically the process we're following for input unions, but not nearly so heavy).

businesses.owner.email

Yeah, the schema-coordinates vs query-path vs result-path distinction is still a little weird... they're definitely distinct but they feel like they ought to be related somehow.

Case in point, in the Shopify example I shared above we're technically sharing a weird combination of coordinate and path. For use of deprecated fields we share the plain coordinate Type.field but for use of deprecated inputs we append the full input path (so not InputType.field but actually Type.field.argument.inputFieldPath....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following yesterday's WG (and the thumbs up above) I believe that @eapache is happy to go ahead with the parenthesized syntax.

Additional note; I wrote Foo.bar(baz:.qux) above; but I think Foo.bar(baz.qux:) has better aesthetics.

Copy link

@cheapsteak cheapsteak Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bit of a tangent (although it seems like using parens is resolved so it may be safe) -

In the example of Foo.bar(baz:).qux, is the contents of the parens carrying useful information for the coordinates?

Assuming arguments require parens, would Foo.bar(baz:).qux not point to the same place as Foo.bar.qux?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it too late to add my opinion? I agree with @eapache the C struct.member notation is much better for several reasons.

    1. Exactly that, it is C style syntax so it comes naturally for many devs who work with C style languages such as php.
    1. A period is easier and quicker to type than (:).
    1. It is 3 non alpha numeric characters instead of 1, which makes a 3 byte difference AND requires a parser while the struct syntax could be implemented in C as a struct without parsing.

This also makes a big difference in embedded and IOT systems which can benefit immensely from graph data structures if they are implemented efficiently. This could literally keep me from using graphQL in a project.

Lastly why call it coordinates? It doesn't have anything to do with position or am I missing something. This constant reinvention of terms in web frameworks has always been a major issue for newcomers. Things should be named for clarification and not because they sound fancy. It is not descriptive of it's purpose. It is used to identify just as it was described in the OP. So why not call it a Field Identifier? Field Descriptor? It may not sound hip or new but I know what it means.

If you go up to non GraphQL literate person you should be able to explain a query without having to explain what a "field coordinate" is after using the word. If you were to ask me to identify the coordinate in those queries I would think you are trolling me and I have been a web dev for 15+ years. I just asked my GF what the Descriptor was and she said "the name after the period". She is a English teacher and doesn't know what a div tag is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had some time to chew this over - given discussions last WG meeting, I'm personally pretty settled on Foo.bar(baz:). That's the option I'm proposing in the RFC.

I've captured this discussion in the RFC here: https://github.com/magicmark/graphql-spec/blob/add_field_coordinates/rfcs/SchemaCoordinates.md#field-arguments

Please let me know if anyone feels I missed anything, or if there's more points to add (or existing points to contest!)

Thanks everyone who has helped so far in thinking through this :)

@excitedbox to answer some stuff:

  1. On the topic of argument syntax, it is a bit of a tradeoff! I've tried to capture the reasoning in a new section in the RFC. I welcome your followup feedback!
  2. For naming, I'll point again to the RFC, as there's a whole section on this. I will say that in the last WG meeting, I literally bought up the point "schema coordinates sounds a bit fancy", but I think (1) ultimately it's a more accurate name than the alternatives I had, and (2) it's reusing prior art (graphql java already used the "coordinates" naming). If you have an alternate suggestion, I'd love to hear your thoughts.

Copy link
Contributor Author

@magicmark magicmark Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm going to leave this conversation thread open till at least the next WG meeting to make sure this discussion remains visible for folks)

Co-authored-by: Benjie Gillam <benjie@jemjie.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 19, 2020

CLA Signed

The committers are authorized under a signed CLA.

@magicmark
Copy link
Contributor Author

(Just signed the CLA)

Co-authored-by: Benjie Gillam <benjie@jemjie.com>
@benjie
Copy link
Member

benjie commented Nov 20, 2020

@brianwarner Should the EasyCLA comment have been automatically updated in this case?

Nevermind, it seems to be fine now.

@benjie
Copy link
Member

benjie commented Dec 8, 2020

Slightly off topic; other ideas for extensions that might build on this syntax:

  • Permalinks for GraphiQL Documentation sidebar ?docs=User.friends>latestMedia>Post.title (the Post. here might represent picking a type (Post) from a union/interface (Media); we could use any number of syntaxes for this so don't focus too much on that 馃槈 )
  • Linking from a field's deprecation reason to the new path to use for this property

    The Query.branchesFromFork field is being removed; please use the following path instead: Query>repositories>forks>branches(matching:)

  • Indicating how to navigate to a specific schema coordinate via a path; e.g. when viewing the documentation for User.firstName GraphiQL might indicate the shortest paths it can be accessed from:
    • Query.me>firstName
    • Query.articles>author>firstName
    • Query.searchMedia>Book.author>firstName
  • Tracking usage counters for each schema coordinate, and the path that's used to access it: counters['Query.cities>libraries>findBook(isbn:)']++

@acao
Copy link
Member

acao commented Dec 31, 2020

worth noting that the GraphiQL docs component already uses a simple dot notation internally for state, so we added url arguments for this to the 2.x rewrite this spring!

also, we authored the hover dot notation that apollo uses in studio, it comes from monaco-graphql/the LSP

@magicmark
Copy link
Contributor Author

@acao could you clarify what is "hover dot notation"?

@magicmark
Copy link
Contributor Author

magicmark commented Jan 4, 2021

I've updated the RFC to capture discussion about field arguments.

I think this was the main thing we had to square away from the last WG meeting.

I think we're in the "dotting the I's" phase at this point, so if there's any lingering feedback (high level, or on the specifics of what's being proposed), I'd love to hear any thoughts, so we can move towards consensus and ultimately merge this.

(cc @benjie @eapache @IvanGoncharov @mjmahone as folks who have specifically expressed interest in this)

Thanks to everyone who has looked at this and given feedback so far :)

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 馃憤

rfcs/SchemaCoordinates.md Outdated Show resolved Hide resolved
rfcs/SchemaCoordinates.md Outdated Show resolved Hide resolved
magicmark and others added 2 commits January 4, 2021 09:34
Co-authored-by: Benjie Gillam <benjie@jemjie.com>
@leebyron leebyron added 馃摚 RFC document PR creates or changes document inside "rfc" folder and removed 馃挕 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) labels Jan 7, 2021
@leebyron leebyron merged commit 83feb76 into graphql:master Jan 7, 2021
brucelongmore28 added a commit to brucelongmore28/graphql-wg-membership-note that referenced this pull request Dec 1, 2022
馃憢 hi!

Would love to get some discussion going on graphql/graphql-spec#735 / graphql/graphql-spec#746 so I can figure out what (or rather "if") what the next steps might be

thanks!

Co-authored-by: Lee Byron <lee@leebyron.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃摚 RFC document PR creates or changes document inside "rfc" folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet