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 BatchHttpLink regression that silently discarded some pending queries #9793

Merged
merged 6 commits into from
Jun 7, 2022

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 7, 2022

This PR attempts to fix both #9690 and #9773 by first reverting PR #9248 (which seems to be the source of the problems with BatchHttpLink), then publishing @apollo/client@3.7.0-beta.2 for comparison purposes, then un-reverting #9248 and instead tweaking the unsubscription logic to make all the tests pass (with most of PR #9428 still applied), then publishing @apollo/client@3.7.0-beta.3 with the final changes, so folks can try/test both versions on the release-3.7 branch before we release these changes in the next v3.6.x release (hence the v3.6.x patch releases milestone).

benjamn added 5 commits June 7, 2022 14:00
…cel"

This reverts commit cefd24c, reversing
changes made to d98f1de.

I plan to publish this version in the next v3.7.0 beta release, then
immediately reinstate most of this functionality, with a narrower
attempt to solve issue #9773 in a follow-up beta release, to allow folks
to compare the behavior of those two versions.
@benjamn benjamn self-assigned this Jun 7, 2022
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Looks great @benjamn - thanks!

benjamn added a commit that referenced this pull request Jun 7, 2022
benjamn added a commit that referenced this pull request Jun 10, 2022
PR #9793 was first released in v3.7.0-beta.3 for testing, and now (in
this PR) will be backported to the `main` branch, to be released in the
next v3.6.x patch version, fixing a `BatchHttpLink` regression
introduced in v3.6.0 that silently discarded some pending queries.

Evidence this worked:
- #9773 (comment)
- #9690 (comment)
- #9690 (comment)
- #9690 (comment)
- #9690 (comment)
benjamn added a commit that referenced this pull request Jun 10, 2022

userEvent.click(screen.getByRole('button', { name: /mutate/i }));

await screen.findByText('item 3');
Copy link
Member Author

@benjamn benjamn Jun 10, 2022

Choose a reason for hiding this comment

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

This test was heavily inspired by the one provided by @skrivle in #9773.

This screen API is new to me. I'll have to try that elsewhere!

Copy link
Contributor

Choose a reason for hiding this comment

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

Glad I was able to help! Testing Library is great, definitely worth checking it out!

benjamn added a commit that referenced this pull request Jun 10, 2022
@vieira
Copy link

vieira commented Oct 18, 2022

@benjamn this issue seems to be back with the release of 3.7.0. Some queries are hanging indefinitely in the loading state.

Replacing the BatchHttpLink with HttpLink or downgrading to 3.6.10 makes the problem go away.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants