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: Initialize query on updateObservableQuery even if skip is true #6999

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

mu29
Copy link
Contributor

@mu29 mu29 commented Sep 10, 2020

Solves #6816, #6796

If skip option is true at the first rendering, the initializeObservableQuery will not be triggered. And even if the skip value changes to false later, calling refetch throws the following error:

Uncaught TypeError: Cannot read property 'refetch' of undefined
    at QueryData._this.obsRefetch

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

@benjamn benjamn changed the base branch from main to release-3.2 September 10, 2020 22:25
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.

This makes sense to me, but I'd like to get @hwillson's take, since he added that return case in #6752. In the meantime, I will merge this into release-3.2 and put out another beta release for you to try. Also happy to port this change back to 3.1.x if that's helpful.

@benjamn benjamn added this to the Post 3.0 milestone Sep 10, 2020
@benjamn benjamn merged commit 59e9049 into apollographql:release-3.2 Sep 10, 2020
@benjamn benjamn mentioned this pull request Sep 10, 2020
11 tasks
@benjamn
Copy link
Member

benjamn commented Sep 10, 2020

@mu29 This change is available now in @apollo/client@3.2.0-beta.12, if you want to give it a try!

@hwillson
Copy link
Member

Looks good to me @mu29 @benjamn - we really just need that line to be called anytime before setOptions; I added it to the top to short circuit things a bit faster, but clearly that was the wrong decision. Thanks!

@mu29
Copy link
Contributor Author

mu29 commented Sep 11, 2020

Thank you for your quick response! @benjamn @hwillson 👍

@watadarkstar
Copy link

watadarkstar commented Sep 25, 2020

Might work on patching this for v3.1.5 as well! Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants