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.

Sign up for GitHub

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

Jump to bottom

Add coordinate field to schema element definitions #3808

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add coordinate field to schema element definitions #3808

wants to merge 2 commits into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Dec 30, 2022 •
edited
Loading

#3145 rebased on main.

depends on #3807

@leebyron comments from original PR:

  • Defines a GraphQLSchemaElement base class which defines a .coordinate property and toString/toJSON methods.
  • Adds base class to types, fields, arguments, input fields, enum values, and directives.
  • Uses this in validation error printing string templates.

Results in a net-reduction of code, minor simplification of definition.ts, and simplification/standardization of error messages

@yaacovCR yaacovCR added the PR: polish 💅 PR doesn't change public API or any observed behaviour label Dec 30, 2022
@netlify
Copy link

netlify bot commented Dec 30, 2022 •
edited
Loading

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 163918f
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/672b53adca8de30008be3a60
😎 Deploy Preview https://deploy-preview-3808--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR yaacovCR mentioned this pull request Dec 31, 2022
Merged
@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch 3 times, most recently from cc309b6 to 39b5de5 Compare January 1, 2023 19:35
@yaacovCR yaacovCR mentioned this pull request Jan 2, 2023
Merged
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jan 2, 2023 •
edited
Loading

@IvanGoncharov @leebyron

While rebasing/reviewing this, I noticed that the new .toConfig() methods return actual *Config types rather than *NormalizedConfig types.

I initially thought to just add them in, but then I also noticed that some of the existing *NormalizedConfig do not carry all of the type parameters from the original *Config types, and I was unsure why and therefore unsure of how to proceed with the new and potentially generic *NormalizedConfig types.

Questions:

  1. Must the new .toConfig() methods return *NormalizedConfig types?
  2. What is the right approach to type parameters with *NormalizedConfig types?

@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch 3 times, most recently from f7e4d06 to e08f1d7 Compare February 6, 2023 12:33
@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch 2 times, most recently from 4dbc894 to 3df173b Compare May 31, 2023 11:07
@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch 3 times, most recently from d2275f2 to 1931581 Compare March 20, 2024 10:44
@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch from 1931581 to d8dc391 Compare August 22, 2024 21:12
@yaacovCR yaacovCR requested a review from a team as a code owner August 22, 2024 21:12
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Aug 23, 2024
@yaacovCR
standardize error messages prior to introducing schema coordinates
37d13e7 
extracted from graphql#3808
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Aug 23, 2024
@yaacovCR
standardize error messages prior to introducing schema coordinates
ccc3dcc 
extracted from graphql#3808
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Aug 23, 2024
@yaacovCR
standardize error messages prior to introducing schema coordinates
fcecd91 
extracted from graphql#3808

PR graphql#3808 uses schema coordinates to improve GraphQL-JS error messages. To reduce the size of the PR, this commit standardizes error messages according to the general pattern that will be introduced with schema coordinates without introducing the coordinates themselves, in the hopes of aiding review of the later PR.

Parenthetically, extracting these changes out of graphql#3808 demonstrates how the use of schema coordinates improves our error messages. For example, it is difficult when printing an error message regarding an argument to easily look up the field of that argument, and, if it a nested input field, the full field path. It is similarly sometimes difficult to know the type of the field or argument triggering the error. Embedding schema coordinates within GraphQL-JS makes this trivial. This PR simply omits the changes from graphql#3808 that are directly enabled by the use of schema coordinates. graphql#3808 can then be rebased on top of this PR to demonstrate the code changes and the direct improvement in the various error messages.
@yaacovCR yaacovCR mentioned this pull request Aug 23, 2024
Merged
yaacovCR added a commit to yaacovCR/graphql-js that referenced this pull request Aug 23, 2024
@yaacovCR
standardize error messages prior to introducing schema coordinates
e184259 
extracted from graphql#3808

PR graphql#3808 uses schema coordinates to improve GraphQL-JS error messages. To reduce the size of the PR, this commit standardizes error messages according to the general pattern that will be introduced with schema coordinates without introducing the coordinates themselves, in the hopes of aiding review of the later PR.

Parenthetically, extracting these changes out of graphql#3808 demonstrates how the use of schema coordinates improves our error messages. For example, it is difficult when printing an error message regarding an argument to easily look up the field of that argument, and, if it a nested input field, the full field path. It is similarly sometimes difficult to know the type of the field or argument triggering the error. Embedding schema coordinates within GraphQL-JS makes this trivial. This PR simply omits the changes from graphql#3808 that are directly enabled by the use of schema coordinates. graphql#3808 can then be rebased on top of this PR to demonstrate the code changes and the direct improvement in the various error messages.
yaacovCR added a commit that referenced this pull request Sep 4, 2024
@yaacovCR
standardize error messages prior to introducing schema coordinates (#…
426b017 
…4177)

extracted from #3808

PR #3808 uses schema coordinates to improve GraphQL-JS error messages.
To reduce the size of the PR, this commit standardizes error messages
according to the general pattern that will be introduced with schema
coordinates without introducing the coordinates themselves, in the hopes
of aiding review of the later PR.

EDITED 8/26/2024:

I was able to reproduce all of the standardized error messages from
#3808 except for the ones in getArgumentValues when it is passed a Field
Definition, because the parent type is not passed. Everything else can
be calculated for the error messages we are currently printing, although
schema coordinates simplifies things.

Extracting these changes out of #3808 and rebasing #3808 on main will
therefore will better demonstrate how schema coordinates improves the
clarity of some of our error messages (namely, getArgumentValues) and
simplifies printing them.
@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch from d8dc391 to 293f34b Compare November 6, 2024 11:25
@leebyron @yaacovCR
Schema Coordinates
9fc6c0b 
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`
@leebyron @yaacovCR
Add coordinate field to schema element definitions
163918f 
* Defines a `GraphQLSchemaElement` base class which defines a `.coordinate` property and `toString`/`toJSON` methods.
* Adds base class to types, fields, arguments, input fields, enum values, and directives.
* Uses this in validation error printing string templates.
@yaacovCR yaacovCR force-pushed the definition-coordinates-rebased branch from 293f34b to 163918f Compare November 6, 2024 11:31
@yaacovCR yaacovCR mentioned this pull request Nov 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leebyron leebyron Awaiting requested review from leebyron

Assignees
No one assigned
Labels
PR: polish 💅 PR doesn't change public API or any observed behaviour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yaacovCR @leebyron