-
Notifications
You must be signed in to change notification settings - Fork 2k
Add support for newer incremental delivery format #8148
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 readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removed
Build ID: 267623d3d32c2c5b9f02e1b2 URL: https://www.apollographql.com/docs/deploy-preview/267623d3d32c2c5b9f02e1b2 |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
7caea09
to
a17ffff
Compare
8478c9e
to
a17ffff
Compare
Ok I think I have this implemented working as I had hoped. 5.1 will now require The only unanswered questions remaining before I think this is 100% ready:
|
MULTIPART_MIXED_EXPERIMENTAL_ALPHA_2: 'multipart/mixed; deferSpec=20220824', | ||
// spec version represents the commit hash of 17.0.0-alpha.9 | ||
MULTIPART_MIXED_EXPERIMENTAL_ALPHA_9: | ||
'multipart/mixed; incrementalDeliverySpec=3283f8a', |
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 lean towards @BoD 's preference from last week of just using deferSpec with a date again. While it might not have been the best design in the first place, it does kinda feel like using the same mechanism twice for the same exact thing ("representing an unreleased incremental delivery format") would be better than changing it? (And then once it's fully standardized, we can drop it.)
interface ObjMap<T> { | ||
[key: string]: T; | ||
} | ||
|
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.
Yeah, I don't know much about TS namespaces and we haven't used them in this project to my knowledge.
packages/server/package.json
Outdated
}, | ||
"peerDependencies": { | ||
"graphql": "^16.11.0" | ||
"graphql": "^16.11.0 || 17.0.0-alpha.9", |
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.
As discussed on Slack, I am somewhat paranoid that this may lead in some cases to npm i @apollo/server
installing [email protected] by default (because it's the dependency of @yaacovr/transform
. I still lean towards us just saying that to support this experimental feature you need to do a manual npm i --legacy-peer-deps
. (What exactly does declaring an optional peer dep win us anyway — enforcement of its version constraint?)
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 exactly does declaring an optional peer dep win us anyway — enforcement of its version constraint?
I think precisely that. Though its a very niche tool so maybe there are more downsides to declaring it as a peer dependency vs not.
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.
Updated in bd5cd63
@@ -0,0 +1,82 @@ | |||
import type { FormattedExecutionResult, GraphQLFormattedError } from 'graphql'; |
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.
General question — now that we're not reordering fields, is it possible we can drop a lot of these types (and just have some higher-up field be unknown
or something)?
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.
Definitely! I'll look through this and see if there are any that are used outside of ordering, but I would definitely love to remove as much as we can.
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.
Discussed over slack. Since it's used by GraphQLResponse
, we're going to leave these so that they are usable. Eventually we'll be able to deprecate these types once graphql
v17 is released and the types can be imported directly from there.
.changeset/fruity-ways-tell.md
Outdated
'@apollo/server': minor | ||
--- | ||
|
||
Apollo Server now supports the modern incremental delivery protocol (`@defer` and `@stream`) that ships with `[email protected]`. To use the modern protocol, clients must send the `Accept` header with a value of `multipart/mixed; incrementalDeliverySpec=3283f8a`. |
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 wonder if "modern" is an overly optimistic term to use throughout this PR. If we come up with a third protocol later, it won't age well. Would "current" work better than "modern"?
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.
This is why I hate naming 😂. I just picked something to differentiate from it and the legacy version. I didn't really find a good name for the "now" version, but I'd be ok with "current" 🙂
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.
Updated in 3c0a88e
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 think the term is also used in errors?
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.
Oops, you're right! Updated in 5d15554
Closes #7932
Adds support for the
[email protected]
@defer
and@stream
incremental delivery protocol.When
[email protected]
is installed, clients must send theAccept
header with a value ofmultipart/mixed; incrementalDeliverySpec=3283f8a
to specify the new format. If theAccept
header is not compatible with the installed version ofgraphql
(such as sendingdeferSpec=20220824
when ``17.0.0-alpha.9` is installed), an error is returned to the client.