-
Notifications
You must be signed in to change notification settings - Fork 2k
Schema Coordinates #3044
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
base: next
Are you sure you want to change the base?
Schema Coordinates #3044
Conversation
c7b87e2
to
0a5630a
Compare
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.
Amazing!
This is way beyond what I was hacking with. Hadn't even considered reusing the existing parser. (I was fiddling around with regexes...)
Thanks for putting this together - this makes a lot of sense.
a4a8920
to
42f9072
Compare
Great feedback - I think I've resolved both:
|
42f9072
to
1399f7c
Compare
3451e3e
to
2f893d6
Compare
1399f7c
to
3cec8c2
Compare
3cec8c2
to
e688f88
Compare
e688f88
to
c68ca9b
Compare
c68ca9b
to
8fa8a4c
Compare
Implements graphql/graphql-spec#794 Adds: * DOT punctuator in lexer * Improvements to lexer errors around misuse of `.` * Minor improvement to parser core which simplified this addition * `SchemaCoordinate` node and `isSchemaCoodinate()` predicate * Support in `print()` and `visit()` * Added function `parseSchemaCoordinate()` since it is a parser entry point. * Added function `resolveSchemaCoordinate()` and `resolveASTSchemaCoordinate()` which implement the semantics (name mirrored from `buildASTSchema`) as well as the return type `ResolvedSchemaElement`
8fa8a4c
to
98e7541
Compare
[#3049 rebased on main](#3049). This is the last rebased PR from the original PR stack concluding with #3049. * Rebased: #3809 [Original: #3092] * Rebased: #3810 [Original: #3074] * Rebased: #3811 [Original: #3077] * Rebased: #3812 [Original: #3065] * Rebased: #3813 [Original: #3086] * Rebased: #3814 (this PR) [Original: #3049] Update: #3044 and #3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from #3049, the original PR: > Implements [graphql/graphql-spec#793](graphql/graphql-spec#793) > > * BREAKING: Changes default values from being represented as an assumed-coerced "internal input value" to a pre-coerced "external input value" (See chart below). > This allows programmatically provided default values to be represented in the same domain as values sent to the service via variable values, and allows it to have well defined methods for both transforming into a printed GraphQL literal string for introspection / schema printing (via `valueToLiteral()`) or coercing into an "internal input value" for use at runtime (via `coerceInputValue()`) > To support this change in value type, this PR adds two related behavioral changes: > > * Adds coercion of default values from external to internal at runtime (within `coerceInputValue()`) > * Removes `astFromValue()`, replacing it with `valueToLiteral()` for use in introspection and schema printing. `astFromValue()` performed unsafe "uncoercion" to convert an "Internal input value" directly to a "GraphQL Literal AST", where `valueToLiteral()` performs a well defined transform from "External input value" to "GraphQL Literal AST". > * Adds validation of default values during schema validation. > Since assumed-coerced "internal" values may not pass "external" validation (for example, Enum values), an additional validation error has been included which attempts to detect this case and report a strategy for resolution. > > Here's a broad overview of the intended end state: > >  --------- Co-authored-by: Lee Byron <lee@leebyron.com>
…QLEnumValue (#4288) this extracts logic from #3044 and #3145 (later rebased as #3807 and #3808) to implement more informative error messages without implementing [the full schema coordinate RFC](graphql/graphql-spec#794) This is a BREAKING CHANGE because these schema elements are now longer plain objects and function differently in various scenarios, for example with `String(<schemaElement>` `JSON.stringifu(<schemaElement>` and `.toString()` and `.toJSON()` --------- Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
I've merged the latest |
// Schema coordinates | ||
export { | ||
resolveSchemaCoordinate, | ||
resolveASTSchemaCoordinate, |
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.
We should make a note about documenting this when we start on the v17 documentation
resolveSchemaCoordinate, | ||
resolveASTSchemaCoordinate, |
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.
Those functions are defined in the spec (see graphql/graphql-spec#794 (comment)), should we put them in language
instead of utlilities
?
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.
fwiw, I vote for utilities
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, it could fit better into the type
section or its own 'coordinate' section.
@benjie I don't have write access to this PR since I wasn't the original author -- do you think we could merge in #4422 and then magicmark#1 to collapse the stack for easier review? (Alternatively, i'm happy to make a yet-another new PR to collapse everything, was trying to avoid making things even more confusing though.) |
+1 on having a single PR for all changes. |
@magicmark let's merge magicmark#1 into #4422 (you have to do that since you own that repository), then incorporate changes to support magicmark/graphql-spec#1, then I can merge the whole lot into this PR assuming there's no pushback. |
type: schema.getType('String'), | ||
}); | ||
|
||
expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(undefined); |
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.
should this be null
?
expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(undefined); | |
expect(resolveSchemaCoordinate(schema, 'private')).to.deep.equal(null); |
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.
per @benjie's comment here magicmark#1 (comment) I believe undefined
is correct, despite the spec saying "null". I think a note somewhere in the spec text saying something like this:
"Algorithms for functions in this spec may return "null" (in a non-error state). In the reference graphql-js implementation, this corresponds to a value of
undefined
"
would be a helpful clarification - so far it's caught us both out :P
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.
Right, it'd help a lot 👍 Also, could we include the reason why we're doing this? Is there any advantage of using undefined
instead of just null
?
This PR is applied to the `schema-coordinates` branch PR here: #3044 Implements schema coordinate spec changes per the June 5th 2025 WG discussion graphql/graphql-spec#794 - Add support for meta-fields (e.g. `__typename`) - Add support for introspection types - Revert back from FieldCoordinate+ValueCoordinate -> MemberCoordinate cc @benjie
resolveSchemaCoordinate, | ||
resolveASTSchemaCoordinate, |
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.
fwiw, I vote for utilities
Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>
UpdateThe 2nd member of this stack by @leebyron was #3145, with that PR adding a coordinate field to all schema elements, allowing library users to move from schema element => to coordinate (as opposed to this PR, which allows the reverse, from coordinate => to schema element. I have now updated #3808 to be rebased on top of this PR with this PR as the base. We can still merge it separately. Point for discussionNow that we have allowed meta-fields in this PR, the other PR #3145/#3808 demonstrates that the schema element <==> coordinate relationship will not allow cycles with meta-fields. For example, coordinate To preserve the symmetry we have with non-meta-fields, we would have to make Other options would be to simply acknowledge this asymmetry:
|
Depends on #3115
Implements graphql/graphql-spec#794
Adds:
.
SchemaCoordinate
node andisSchemaCoodinate()
predicateprint()
andvisit()
parseSchemaCoordinate()
since it is a parser entry point.resolveSchemaCoordinate()
andresolveASTSchemeCoordinate()
which implement the semantics (name mirrored frombuildASTSchema
) as well as the return typeGraphQLSchemaElement