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

fix includeUnusedVariables not working with BatchHttpLink #10754

Conversation

sincraianul
Copy link
Contributor

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@apollo-cla
Copy link

@sincraianul: 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 Apr 10, 2023

‼️ Deploy request for apollo-client-docs rejected.

Name Link
🔨 Latest commit 5e4c894

@changeset-bot
Copy link

changeset-bot bot commented Apr 10, 2023

🦋 Changeset detected

Latest commit: b301242

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@osdiab
Copy link

osdiab commented Apr 10, 2023

this closes #8824

@sincraianul
Copy link
Contributor Author

@benjamn @alessbell @bignimbus @hwillson @jerelmiller @phryneas Would be awesome to get a release for this ASAP as it's something critical to our use case. Thanks!

@alessbell
Copy link
Member

Hi @sincraianul 👋

Thanks for taking the time to look into this and opening a PR!

While an argument can be made for targeting this at a patch release (since the BatchHttpLink has long included includeUnusedVariables in its options type), since this will add functionality and do a light refactor re: extracting filterOperationVariables (and since we're in the home stretch before releasing the next minor, 3.8 🎉) we'd like to target this at the release-3.8 branch.

I'll make that change and we'll get this reviewed as soon as we can. Thanks!

@alessbell alessbell changed the base branch from main to release-3.8 April 12, 2023 15:15
@alessbell alessbell force-pushed the fix-batch-http-includeUnusedVariables branch from a262d59 to 26e0aa0 Compare April 12, 2023 16:36
@alessbell alessbell added 🏓 awaiting-team-response requires input from the apollo team 🧞 fixed-by-contributor issues where the contributor went the extra mile and fixed the problem with a PR 🧶 minor-release labels Apr 12, 2023
@jerelmiller jerelmiller linked an issue Apr 13, 2023 that may be closed by this pull request
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing this @sincraianul 🔥 🔥. We will make sure this goes out with the next v3.8.0-alpha.x release!

@jerelmiller jerelmiller merged commit 64b3048 into apollographql:release-3.8 Apr 13, 2023
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2023
@sincraianul sincraianul deleted the fix-batch-http-includeUnusedVariables branch November 24, 2023 16:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🏓 awaiting-team-response requires input from the apollo team 🧞 fixed-by-contributor issues where the contributor went the extra mile and fixed the problem with a PR 🧶 minor-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

includeUnusedVariables support for BatchHTTPLink
5 participants