Skip to content

Spec edits for incremental delivery, Section 3 only #1132

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

Open
wants to merge 8 commits into
base: incremental-integration
Choose a base branch
from

Conversation

robrichard
Copy link
Contributor

@robrichard robrichard commented Jan 7, 2025

Extracted from the full PR (#1110) and targeting an integration branch to aid in review.

Helpful reference material:

Response format examples: graphql/defer-stream-wg#69
Glossary: graphql/defer-stream-wg#106
GraphQL Conf talk: https://www.youtube.com/watch?v=LEyDeNoobT0

Response types

/** Currently defined types */
type RequestErrorResult = {
  errors: Error[] // request errors only
  extensions?: object
}
type ExecutionResult = {
  data: {} | null
  errors?: Error[] // execution errors only
  extensions?: object
}
type ResponseStream = ExecutionResult[]

/** New types that are yielded by an Incremental Stream */
interface InitialExecutionResult extends ExecutionResult {
  incremental: IncrementalResult[]
  pending?: Pending[]
  completed?: CompletedResult[],
  hasNext?: boolean;
}

type SubsequentExecutionResult = {
  incremental?: IncrementalResult[]
  pending?: Pending[]
  completed?: CompletedResult[],
  hasNext?: boolean;
}

type IncrementalStream = [InitialExecutionResult, ...SubsequentExecutionResult[]];

/** Response is updated to include IncrementalStream as a new member */
type Response = RequestErrorResult | ExecutionResult | ResponseStream | IncrementalStream;

type IncrementalResult = IncrementalObjectResult | IncrementalListResult

yaacovCR pushed a commit to graphql/graphql-js that referenced this pull request Jan 12, 2025
Updated to reflect spec draft
graphql/graphql-spec#1132

Also changed the argument order to match the spec draft
@robrichard robrichard requested a review from benjie January 30, 2025 19:56
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.

Lots of nit-picky comments but I think we're pretty close! Do we have a glossary somewhere? I think we need to be really crisp on terms like "result", "response", "payload" and the like.

Comment on lines 2188 to 2189
responses: the initial response containing all non-deferred data, while
subsequent responses include deferred data.
Copy link
Member

Choose a reason for hiding this comment

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

I believe that we've refined the terminology around this: there's one response. The response for stream/defer is a stream consisting of an initial result payload followed by a number of incremental payloads. I couldn't find where we discussed this, so please correct as appropriate.

Suggested change
responses: the initial response containing all non-deferred data, while
subsequent responses include deferred data.
payloads: the initial payload containing all non-deferred data, while subsequent
payloads include deferred data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjie after the last discussion we had, I decided to drop payload. The response section now says:

The result of a GraphQL request must be either a single initial response or an
incremental stream. The response will be an incremental stream when the GraphQL
service has deferred or streamed data as a result of the @defer or @stream
directives. When the result of the GraphQL operation is an incremental stream,
the first value will be an initial response, followed by one or more subsequent
responses.

Comment on lines 2255 to 2257
- `if: Boolean! = true` - When `true`, field _should_ be streamed (see related
note below). When `false`, the field must not be streamed and all list items
must be initially included. Defaults to `true` when omitted.
Copy link
Member

Choose a reason for hiding this comment

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

The "and" here implies this is an additional behaviour

Suggested change
- `if: Boolean! = true` - When `true`, field _should_ be streamed (see related
note below). When `false`, the field must not be streamed and all list items
must be initially included. Defaults to `true` when omitted.
- `if: Boolean! = true` - When `true`, field _should_ be streamed (see related
note below). When `false`, the field must behave as if the `@stream` directive
is not present—it must not be streamed and all of the list items must be
included. Defaults to `true` when omitted.

@robrichard robrichard force-pushed the incremental-integration-type-system branch from d5322ae to 32785b8 Compare February 7, 2025 20:33
@robrichard
Copy link
Contributor Author

@benjie I added a rough glossary here: graphql/defer-stream-wg#106, I'll keep refining it as we go

@robrichard robrichard requested a review from benjie February 24, 2025 19:38
@robrichard robrichard force-pushed the incremental-integration-type-system branch 2 times, most recently from 176172f to 7c0ba73 Compare February 24, 2025 19:42
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

These changes look correct to me, I don't think the definition of the @stream/@defer directives have changed in a very long time, so this feels good.

@robrichard robrichard force-pushed the incremental-integration-type-system branch from cf072a4 to 47f362c Compare March 7, 2025 17:07
@robrichard robrichard force-pushed the incremental-integration-type-system branch from 0640179 to b3187e0 Compare April 3, 2025 11:47
@robrichard robrichard changed the base branch from incremental-integration to main April 11, 2025 16:06
@robrichard robrichard changed the base branch from main to incremental-integration April 11, 2025 16:06
@robrichard robrichard force-pushed the incremental-integration-type-system branch from b3187e0 to 3b8799e Compare July 1, 2025 16:06
@robrichard robrichard requested a review from Keweiqu July 1, 2025 16:07
@robrichard robrichard force-pushed the incremental-integration-type-system branch from 3b8799e to 64dc38f Compare July 1, 2025 16:08
Comment on lines +2225 to +2253
initialCount: Int! = 0
if: Boolean! = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

keep if consistently as the first arg


#### @stream Arguments

- `initialCount: Int! = 0` - The number of list items to include initially when
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this arg be last? to match if/label/* order from defer

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.

6 participants