-
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
Conversation
✅ Docs preview ready
The preview is ready to be viewed. View the preview
File Changes
0 new, 1 changed, 0 removed
* (developer-tools)/apollo-server/(latest)/workflow/requests.md
Build ID: afe05c62ab65b9d1a52c8666
Build Logs: View logs
URL: https://www.apollographql.com/docs/deploy-preview/afe05c62ab65b9d1a52c8666
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.
Ok I think I have this implemented working as I had hoped. 5.1 will now require 17.0.0-alpha.9 and will be able to support the legacy format if the @yaacovcr/transform package is installed. This allows Apollo Server to simultaneously support both the old and new formats (which means clients can switch over whenever).
The only unanswered questions remaining before I think this is 100% ready:
- What do we want the
acceptheader paramter value to be? (Add support for newer incremental delivery format #8148 (comment) ) - How do we want to change/expose the types? Do we want to use a namespace, use the
Alpha<n>naming scheme, or something else? (Add support for newer incremental delivery format #8148 (comment) ) - Do we want to get rid of field ordering? (Add support for newer incremental delivery format #8148 (comment) )
packages/server/src/ApolloServer.ts
Outdated
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.)
packages/server/package.json
Outdated
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 graphql@17.0.0-alpha.9 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
.changeset/fruity-ways-tell.md
Outdated
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
Co-authored-by: David Glasser <glasser@apollographql.com>
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 you've addressed all my concerns from last time.
I'm not really sure what the v0.1 refers to but it's as good as anything else. Have you tested what happens when both deferSpec and incrementalDeliverySpec are sent? Would be good to make that well-defined, but not necessary.
This isn't a regression, but it might be nice to be explicit somewhere that --legacy-peer-deps is required for installing the alpha and transform.
🙌
I'm not really sure what the v0.1 refers to but it's as good as anything else.
We're going to document the spec as it exists today in specs.apollo.dev. @calvincestari has a PR up right now to add this: apollographql/specs#67
That v0.1 refers to the version in that PR. We figure this is a good way to reference the spec as implemented in that version. If new ones come along, we can add v0.2, 0.3, etc for as long as we need until the spec is stable. But we plan to have those versions documented to capture the spec as it exists at the time we add support for the version.
Have you tested what happens when both deferSpec and incrementalDeliverySpec are sent?
Actually no! I'll try this out. I believe it will return the newer version of the spec (since it uses Negotiator and prefers that version), but wouldn't hurt to add an explicit test.
This isn't a regression, but it might be nice to be explicit somewhere that --legacy-peer-deps is required for installing the alpha and transform.
Good call. I'll see if I can update the docs page and include that.
Uh oh!
There was an error while loading. Please reload this page.
Closes #7932
Adds support for the
graphql@17.0.0-alpha.9@deferand@streamincremental delivery protocol.When
graphql@17.0.0-alpha.9is installed, clients must send theAcceptheader with a value ofmultipart/mixed; incrementalDeliverySpec=graphql/incremental/v0.1to specify the new format. If theAcceptheader is not compatible with the installed version ofgraphql(such as sendingdeferSpec=20220824when ``17.0.0-alpha.9` is installed), an error is returned to the client.