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

Spec edits for incremental delivery, Section 3 & 7 only #1124

Open
wants to merge 6 commits into
base: incremental-integration
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cspell.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ ignoreRegExpList:
- /[a-z]{2,}'s/
words:
# Terms of art
- deprioritization
- endianness
- interoperation
- monospace
Expand Down
106 changes: 104 additions & 2 deletions spec/Section 3 -- Type System.md
Original file line number Diff line number Diff line change
Expand Up @@ -794,8 +794,9 @@ And will yield the subset of each object type queried:
When querying an Object, the resulting mapping of fields are conceptually
ordered in the same order in which they were encountered during execution,
excluding fragments for which the type does not apply and fields or fragments
that are skipped via `@skip` or `@include` directives. This ordering is
correctly produced when using the {CollectFields()} algorithm.
that are skipped via `@skip` or `@include` directives or temporarily skipped via
`@defer`. This ordering is correctly produced when using the {CollectFields()}
algorithm.
Comment on lines +797 to +799
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
that are skipped via `@skip` or `@include` directives or temporarily skipped via
`@defer`. This ordering is correctly produced when using the {CollectFields()}
algorithm.
that are skipped via `@skip` or `@include` directives or postponed via `@defer`
or `@stream`. This ordering is correctly produced when using the
{CollectFields()} algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Small tweak: in this context, I think only defer is relevant as a cause for fields being postponed. (Stream may cause subfields to be postponed, but there would always be an empty list for the initial field.)


Response serialization formats capable of representing ordered maps should
maintain this ordering. Serialization formats which can only represent unordered
Expand Down Expand Up @@ -1942,6 +1943,11 @@ by a validator, executor, or client tool such as a code generator.

GraphQL implementations should provide the `@skip` and `@include` directives.

GraphQL implementations are not required to implement the `@defer` and `@stream`
directives. If either or both of these directives are implemented, they must be
implemented according to this specification. GraphQL implementations that do not
support these directives must not make them available via introspection.

robrichard marked this conversation as resolved.
Show resolved Hide resolved
GraphQL implementations that support the type system definition language must
provide the `@deprecated` directive if representing deprecated portions of the
schema.
Expand Down Expand Up @@ -2162,3 +2168,99 @@ to the relevant IETF specification.
```graphql example
scalar UUID @specifiedBy(url: "https://tools.ietf.org/html/rfc4122")
```

### @defer

```graphql
directive @defer(
label: String
if: Boolean! = true
) on FRAGMENT_SPREAD | INLINE_FRAGMENT
```

The `@defer` directive may be provided for fragment spreads and inline fragments
to inform the executor to delay the execution of the current fragment to
indicate deprioritization of the current fragment. A query with `@defer`
directive will cause the request to potentially return multiple responses, where
non-deferred data is delivered in the initial response and data deferred is
delivered in a subsequent response. `@include` and `@skip` take precedence over
`@defer`.

```graphql example
query myQuery($shouldDefer: Boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

This falls foul of the CoerceArgumentValues() bug that I've proposed we fix previously. Also, since this variable is nullable you'd expect to be able to set it to null, but doing so would be an error. Let's give it a default here so as to not introduce more problematic examples into the spec.

Suggested change
query myQuery($shouldDefer: Boolean) {
query myQuery($shouldDefer: Boolean! = true) {

user {
name
...someFragment @defer(label: "someLabel", if: $shouldDefer)
}
}
fragment someFragment on User {
id
profile_picture {
uri
}
}
```

#### @defer Arguments

- `if: Boolean! = true` - When `true`, fragment _should_ be deferred (see
related note below). When `false`, fragment will not be deferred and data will
be included in the initial response. Defaults to `true` when omitted.
robrichard marked this conversation as resolved.
Show resolved Hide resolved
- `label: String` - May be used by GraphQL clients to identify the data from
responses and associate it with the corresponding defer directive. If
provided, the GraphQL service must add it to the corresponding pending object
in the response. `label` must be unique label across all `@defer` and
`@stream` directives in a document. `label` must not be provided as a
variable.
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
- `label: String` - May be used by GraphQL clients to identify the data from
responses and associate it with the corresponding defer directive. If
provided, the GraphQL service must add it to the corresponding pending object
in the response. `label` must be unique label across all `@defer` and
`@stream` directives in a document. `label` must not be provided as a
variable.
- `label: String` - An optional string literal (variables are disallowed) used
by GraphQL clients to identify data from responses and associate it with the
corresponding defer directive. If provided, the GraphQL service must include
this label in the corresponding pending object within the response. The
`label` argument must be unique across all `@defer` and `@stream` directives
in the document.


### @stream

```graphql
directive @stream(
label: String
if: Boolean! = true
initialCount: Int = 0
) on FIELD
```

The `@stream` directive may be provided for a field of `List` type so that the
backend can leverage technology such as asynchronous iterators to provide a
partial list in the initial response, and additional list items in subsequent
responses. `@include` and `@skip` take precedence over `@stream`.
Copy link
Contributor

Choose a reason for hiding this comment

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

do you want to add a comment about validation error if this directive is applied on a non-list type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a sentence mentioning the "Stream Directives Are Used On List Fields" rule. It's a broken link for now, but we have this rule defined in the full PR


```graphql example
query myQuery($shouldStream: Boolean) {
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
query myQuery($shouldStream: Boolean) {
query myQuery($shouldStream: Boolean! = true) {

user {
friends(first: 10) {
nodes @stream(label: "friendsStream", initialCount: 5, if: $shouldStream)
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this needs a selection set?

Suggested change
nodes @stream(label: "friendsStream", initialCount: 5, if: $shouldStream)
nodes @stream(label: "friendsStream", initialCount: 5, if: $shouldStream) {
name
}

Also should this example come with a schema?

}
}
}
```

#### @stream Arguments

- `if: Boolean! = true` - When `true`, field _should_ be streamed (see related
note below). When `false`, the field will not be streamed and all list items
will be included in the initial response. Defaults to `true` when omitted.
- `label: String` - May be used by GraphQL clients to identify the data from
responses and associate it with the corresponding stream directive. If
provided, the GraphQL service must add it to the corresponding pending object
in the response. `label` must be unique label across all `@defer` and
`@stream` directives in a document. `label` must not be provided as a
variable.
- `initialCount: Int` - The number of list items the service should return as
part of the initial response. If omitted, defaults to `0`. A field error will
be raised if the value of this argument is less than `0`.

Note: The ability to defer and/or stream parts of a response can have a
robrichard marked this conversation as resolved.
Show resolved Hide resolved
potentially significant impact on application performance. Developers generally
need clear, predictable control over their application's performance. It is
highly recommended that GraphQL services honor the `@defer` and `@stream`
directives on each execution. However, the specification allows advanced use
cases where the service can determine that it is more performant to not defer
and/or stream. Therefore, GraphQL clients _must_ be able to process a response
that ignores the `@defer` and/or `@stream` directives. This also applies to the
`initialCount` argument on the `@stream` directive. Clients _must_ be able to
process a streamed response that contains a different number of initial list
items than what was specified in the `initialCount` argument.
Comment on lines +2275 to +2278
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I didn't realise we had applied this to initialCount - I thought it was either non-deferred (i.e. give you the whole list) or deferred with initialCount supplied.

I think we should revisit this discussion, it's quite different to the general "don't defer" and "don't stream" optimizations in my mind - specifically if you specify initialCount: 2 I'd argue that at least 2 results should be supplied, or the entire thing should not be deferred (e.g. if there are fewer than 2 results). Skipping @defer / @include result in the client getting more data up front, but ignoring initialCount allows for the client to get less data up front, and that's a problem to my mind.

Loading
Loading