-
Notifications
You must be signed in to change notification settings - Fork 682
Implement @defer "June 2023" response format #6331
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: main
Are you sure you want to change the base?
Conversation
✅ Docs Preview ReadyConfiguration{
"repoOverrides": {
"apollographql/apollo-kotlin@main": {
"remote": {
"owner": "apollographql",
"repo": "apollo-kotlin",
"branch": "defer-incremental-response-format-june-2023"
}
}
}
}
1 pages published. Build will be available for 30 days. |
e2da859
to
bb504f6
Compare
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removed
Build ID: 65212b0a9c9b1f3e8aaf6e6a URL: https://www.apollographql.com/docs/deploy-preview/65212b0a9c9b1f3e8aaf6e6a |
e1ee642
to
4fa99ad
Compare
Apollo Client 4 introduces different handlers for different incremental formats. We could do the same while the format iterates. |
…gmentIdentifier -> IncrementalResultIdentifier
95e0b8c
to
fc4f174
Compare
… configuration. Defaults to the legacy one.
fc4f174
to
8dfc4b3
Compare
...lo-api/src/commonMain/kotlin/com/apollographql/apollo/api/http/DefaultHttpRequestComposer.kt
Outdated
Show resolved
Hide resolved
...lo-api/src/commonMain/kotlin/com/apollographql/apollo/api/http/DefaultHttpRequestComposer.kt
Outdated
Show resolved
Hide resolved
...otlin/com/apollographql/apollo/internal/incremental/Defer20220824IncrementalResultsMerger.kt
Outdated
Show resolved
Hide resolved
.../commonMain/kotlin/com/apollographql/apollo/internal/incremental/IncrementalResultsMerger.kt
Outdated
Show resolved
Hide resolved
this.engine = httpEngine | ||
} | ||
|
||
fun incrementalDeliveryProtocol(incrementalDeliveryProtocol: IncrementalDeliveryProtocol) = apply { |
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.
Missing KDoc here
Also, looks like apollo-client went with incrementalHandler
. Might be worth reusing some of the terminology? But we also have "protocol" for other things like WebSockets so my logaf is low.
public final class com/apollographql/apollo/internal/incremental/Defer20220824IncrementalResultsMerger : com/apollographql/apollo/internal/incremental/IncrementalResultsMerger { | ||
public fun <init> ()V | ||
public fun getHasNext ()Z | ||
public fun getIncrementalResultIdentifiers ()Ljava/util/Set; | ||
public fun getMerged ()Ljava/util/Map; | ||
public fun isEmptyResponse ()Z | ||
public fun merge (Ljava/util/Map;)Ljava/util/Map; | ||
public fun merge (Lokio/BufferedSource;)Ljava/util/Map; | ||
public fun reset ()V | ||
} | ||
|
||
public final class com/apollographql/apollo/internal/incremental/GraphQL17Alpha9IncrementalResultsMerger : com/apollographql/apollo/internal/incremental/IncrementalResultsMerger { |
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.
It's a bit weird that one is Defer20220824
and the other GraphQL17Alpha9
. I haven't followed all the latest discussions lately but could we somwhat uniformized that? Are there specs versions (or tags) somewhere that describe what each one is doing?
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.
It doesn’t look like there are any tags on the spec itself, so using graphql-js versions is cool. So maybe we should rename Defer20220824
to whatever version of graphql-js it corresponds to, for consistency. For now I kept the same naming as Apollo Client, but maybe it’s not too late to change the name there too.
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.
Sounds good. Let's keep a practical approach here and do whatever is easiest. This is all experimental so the whole point is to move fast and iterate 👍
...-runtime/src/commonMain/kotlin/com/apollographql/apollo/network/http/HttpNetworkTransport.kt
Outdated
Show resolved
Hide resolved
* Both `@defer` and `@stream` are supported with this format. | ||
*/ | ||
object GraphQL17Alpha9 : IncrementalDeliveryProtocolImpl { | ||
// TODO To be agreed upon with the router and other clients |
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 need to agree on that.
See also https://github.com/apollographql/apollo-client/pull/12926/files#r2369415433
...-runtime/src/commonMain/kotlin/com/apollographql/apollo/network/http/HttpNetworkTransport.kt
Outdated
Show resolved
Hide resolved
...lo-api/src/commonMain/kotlin/com/apollographql/apollo/api/http/DefaultHttpRequestComposer.kt
Outdated
Show resolved
Hide resolved
...Main/kotlin/com/apollographql/apollo/internal/incremental/IncrementalDeliveryProtocolImpl.kt
Outdated
Show resolved
Hide resolved
libraries/apollo-api/src/commonMain/kotlin/com/apollographql/apollo/api/Executables.kt
Outdated
Show resolved
Hide resolved
payloadResponse != null -> payloadResponse | ||
else -> null | ||
} | ||
} else { |
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.
Is there a way to test from the response whether it supports the incremental delivey protocol? Is there something in response.headers["content-type"]
that signals what protocol was used by the server?
...rc/commonMain/kotlin/com/apollographql/apollo/network/websocket/WebSocketNetworkTransport.kt
Show resolved
Hide resolved
...ollo-runtime/src/commonTest/kotlin/test/defer/GraphQL17Alpha9IncrementalResultsMergerTest.kt
Show resolved
Hide resolved
if (get("accept") == null) { | ||
add( | ||
HttpHeader("accept", | ||
if (apolloRequest.operation is Subscription<*>) { | ||
"multipart/mixed;subscriptionSpec=1.0, application/graphql-response+json, application/json" | ||
} else { | ||
"multipart/mixed;deferSpec=20220824, application/graphql-response+json, application/json" | ||
} | ||
) |
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.
I’ve kept this here because if somebody is using DefaultHttpRequestComposer
but not HttpNetworkTransport
it’s a behavior change (no accept header) - no strong opinion though.
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.
What about application/graphql-response+json, application/json
as a fallback? This is the most standard accept
header. This way DefaultHttpRequestComposer
is isolated from the incremental delivery concerns. That's a behavior change but probably very small blast radius?
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.
Works for me 👍
…efaultHttpRequestComposer
Implements the latest (as of 2025-09-24) @defer incremental response format as seen in examples and spec edits.
A protocol can now be passed to the
HttpNetworkTransport
to choose which to use (legacy20220824
is still the default).