Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Schema Coordinates #746
RFC: Schema Coordinates #746
Changes from 7 commits
72438bf
3c9c8f9
109e3c6
e5ea9b5
fe62f67
3cd6242
9caf719
e423af3
078bcfc
f00ce54
bfe9db9
80f78a3
0ae4b46
da052fe
1f9cfc6
d8396c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
might refer to:
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
vsFoo.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:"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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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 notInputType.field
but actuallyType.field.argument.inputFieldPath...
.There was a problem hiding this comment.
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 thinkFoo.bar(baz.qux:)
has better aesthetics.There was a problem hiding this comment.
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 asFoo.bar.qux
?There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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)