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

Editorial: move "Path" to it's own section #1129

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

Conversation

robrichard
Copy link
Contributor

Extracted from the @defer/@stream spec edits (#1110) to minimize the diff there.

Since the incremental delivery response format also uses Path, I removed it from the Errors section and into it's own section.

Copy link

netlify bot commented Dec 3, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 2087016
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/677fe70d87a7620008d28be8
😎 Deploy Preview https://deploy-preview-1129--graphql-spec-draft.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.

@robrichard robrichard requested a review from benjie January 6, 2025 14:01
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.

Some suggested modifications:

spec/Section 7 -- Response.md Outdated Show resolved Hide resolved
spec/Section 7 -- Response.md Outdated Show resolved Hide resolved
spec/Section 7 -- Response.md Outdated Show resolved Hide resolved
@robrichard
Copy link
Contributor Author

@benjie thanks for the feedback! I have applied your suggestions.

@robrichard robrichard force-pushed the robrichard/error-path branch from f9c4359 to 2087016 Compare January 9, 2025 15:11
@robrichard robrichard requested a review from benjie January 9, 2025 15:11
the path to the error must use the aliased name, since it represents a path in
the response, not in the request.
`null` result is intentional or caused by a runtime error. The value of this
entry is described in the [Path](#sec-Path) section.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entry is described in the [Path](#sec-Path) section.
_path entry_ is described in the [Path](#sec-Path) section.

Comment on lines +243 to +249
:: A _path entry_ allows for the association with a particular field reached
during GraphQL execution. The value for this entry must be a list of path
segments starting at the root of the response and ending with the field to be
associated with. Path segments that represent fields must be strings, and path
segments that represent list indices must be 0-indexed integers. If a path
segment is associated with an aliased field it must use the aliased name, since
it represents a path in the response, not in the request.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:: A _path entry_ allows for the association with a particular field reached
during GraphQL execution. The value for this entry must be a list of path
segments starting at the root of the response and ending with the field to be
associated with. Path segments that represent fields must be strings, and path
segments that represent list indices must be 0-indexed integers. If a path
segment is associated with an aliased field it must use the aliased name, since
it represents a path in the response, not in the request.
:: A _path entry_ is an entry within an _error result_ that allows for
association with a particular field reached during GraphQL execution.
The value for a _path entry_ must be a list of path segments starting at the
root of the response and ending with the field to be associated with. Path
segments that represent fields must be strings, and path segments that represent
list indices must be 0-indexed integers. If a path segment is associated with an
aliased field it must use the aliased name, since it represents a path in the
response, not in the request.

I tried to make this independent of error but I was just not happy with how it read. When you integrate this into the stream/defer work, you can change just the definition, e.g. something like:

:: A _path entry_ is an entry within a _request error_ or _incremental payload_
that allows for association with a particular field reached during GraphQL
execution.

Comment on lines +251 to +252
When the _path entry_ is present on an "Error result", it identifies the
response field which experienced the error.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When the _path entry_ is present on an "Error result", it identifies the
response field which experienced the error.
When the _path entry_ is present on an _error result_, it identifies the
response field which experienced the error.

This references a definition that doesn't yet exist, but that's okay... We can create it later.

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

Successfully merging this pull request may close these issues.

2 participants