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

Added support for new incremental delivery format #7736

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nathan-knight
Copy link

The format for incremental delivery has been updated: graphql/defer-stream-wg#69

I have added the new fields and ensured they get transmitted if they are present. It should still work with the previous format.

It is released in graphql@17.0.0-alpha.3, but I didn't want to disturb the existing integration test that uses it so I haven't added anything for it. In order to maintain the two in parallel the CI test would need to be duplicated to run with this version.

@apollo-cla
Copy link

@nathan-knight: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@netlify
Copy link

netlify bot commented Sep 26, 2023

👷 Deploy request for apollo-server-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 31f0747

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 26, 2023

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.

Latest deployment of this branch, based on commit 31f0747:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

@trevor-scheer
Copy link
Member

@nathan-knight thanks for putting this together. The support for this in Apollo Server is still considered experimental / subject to change for exactly this reason, so I would like the smoke and integration tests to be updated to use the new alpha as well.

I didn't look too closely, but I'd also like for the "polyfill" stuff to match the latest alpha rather than attempt to be compatible with both. i.e. if there's anything in those types we should remove that are no longer relevant let's do that (and maybe you already did, I haven't verified against the graphql-js types or looked too closely at the PR).

@nathan-knight
Copy link
Author

@trevor-scheer I didn't remove the old logic but if removing support for 17.0.0-alpha.1 and 17.0.0-alpha.2 is an option then I can try my hand at updating the tests and removing the outdated parts

@trevor-scheer
Copy link
Member

@nathan-knight yeah I think that's ideal. Right now we reference an old canary build in various places, we should replace all references to those with graphql@17.0.0-alpha.3 and update tests as necessary.

To run the @defer tests locally, you'll need to:

  • temporarily install graphql@17.0.0-alpha.3 at the top level dev dependencies (using --force)
  • run tests with INCREMENTAL_DELIVERY_TESTS_ENABLED=t npm run test

OR

  • (after updating the canary tag in prepare.sh to graphql@17.0.0-alpha.3
  • run INCREMENTAL_DELIVERY_TESTS_ENABLED=t npm run test:smoke

Second option is less messy but the test output won't be as great.

@Maxou44
Copy link

Maxou44 commented Nov 1, 2023

Any news about this PR?

@u873838
Copy link

u873838 commented Aug 2, 2024

What are the blockers on getting this merged?

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.

5 participants