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 useSubscription executes skipped subscription if input changes #9299

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

levrik
Copy link
Contributor

@levrik levrik commented Jan 13, 2022

Checklist:

  • 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

This bug triggered a regression in our application after updating from Apollo Client 3.3.21 to 3.5.7.

The unit test testing the skip flag wasn't actually working correctly. It didn't fail when skip was set to false.
I've updated it to make sure no subscription is being executed by spying on onSetup callback of the link instead of the onSubscriptionData one on the hook which isn't called anyway since simulateResult isn't called as part of the test. I also extended the test to make sure no subscription is being executed when variables are changing.
The final version of the test was verified to fail on main branch.

I wasn't sure if I shall extend the existing test or add a new one. Let me know if a new test case for the update case is preferred.

@apollo-cla
Copy link

@levrik: 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/

@levrik levrik changed the title Fix executing a skipped subscription if variables change Fix useSubscription executing skipped subscription if input changes Jan 13, 2022
@levrik levrik changed the title Fix useSubscription executing skipped subscription if input changes Fix useSubscription executes skipped subscription if input changes Jan 13, 2022
@levrik
Copy link
Contributor Author

levrik commented Jan 21, 2022

Rebased on main branch

@brainkim
Copy link
Contributor

@levrik This is wonderful, though I might need to double check the useEffect() logic once more before merging...

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

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

Thanks for this @levrik!

@brainkim If you have time, could you see if this merges cleanly into release-3.6 after we merge it into main? Also happy to do it if you're busy.

@benjamn benjamn added this to the v3.5.x patch releases milestone Jan 25, 2022
@brainkim brainkim merged commit a565fd5 into apollographql:main Jan 25, 2022
@levrik levrik deleted the fix-skip-subscription branch January 26, 2022 08:23
benjamn added a commit that referenced this pull request Feb 10, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants