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

clearing out errors when fetchMore is called #2535

Closed
wants to merge 1 commit into from

Conversation

prybalko
Copy link

@prybalko prybalko commented Nov 9, 2017

This fixes #2533. Does this seem like a reasonable amendment?

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
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

@apollo-cla
Copy link

@prybalko: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@apollo-cla
Copy link

Fails
🚫

No CHANGELOG added.

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Messages
📖

Please add your name and email to the AUTHORS file (optional)

📖

If this was a change that affects the external API, please update the docs and post a link to the PR in the discussion

Generated by 🚫 dangerJS

@prybalko
Copy link
Author

^

@apollographql apollographql deleted a comment from stale bot Dec 8, 2017
@jbaxleyiii
Copy link
Contributor

@prybalko this may work, can you add a test to show the issue and how this fixes it?

@kennethlynne
Copy link

Please get this in

https://media.giphy.com/media/9GLiEBLM0qiSQ/giphy.gif

@jbaxleyiii
Copy link
Contributor

@kennethlynne would you be able to pick up this PR and add a couple test cases for it?

@sondremare
Copy link
Contributor

I will take a look at it, later this week (maybe tomorrow).

@sondremare
Copy link
Contributor

I still receive the same error even with this fix

Uncaught (in promise) TypeError: Cannot set property 'networkStatus' of undefined
    at QueryStore.initQuery (queries.js:65)
    at QueryManager.fetchQuery (QueryManager.js:181)
    at ObservableQuery.js:163
    at <anonymous>

@sondremare
Copy link
Contributor

sondremare commented Jan 17, 2018

I need some help here @jbaxleyiii. This is how I have observed fetch-more to work in apollo.

  1. User opens page with associated query. Query is given an id 1, and fetched from backend
  2. User wants to fetch more. Query is given a new id of 2, and the fetchMoreForQueryId is set to the original query's id, namely 1.
  3. Fetch more fails due to spotty/no network.
    4a) User regains network and tries to fetch more again. Null pointers and hell breaks loose. Query is unable to recover
    4b) User is still without network and tries to fetch more again, same thing occurs. Null pointers and hell breaks loose.

Not having as much intimate knowledge of the workings of apollo, I am unsure of how to recover here. When fetch more fails. should we reassign the original query back to query id 1, since we have that in cache. And only error out the fetchMore (query with id 2)? Or is there something major/minor I am missing here?

Reproducible repo for apollo 2 here: https://github.com/sondremare/react-apollo-fetch-more-error

@jbaxleyiii
Copy link
Contributor

@sondremare I'll take a look! This is definitely something that needs to be fixed!

@sondremare
Copy link
Contributor

@jbaxleyiii You probably have seen it, but I have a PR here #2906 that seems to fix the problem if used alongside react-apollo 2.1.0-beta, according to discussion in #2539

@hwillson
Copy link
Member

As mentioned in #2533 (comment), this is no longer an issue (verified against apollo-client 2.3.1). Thanks very much for working on this!

@hwillson hwillson closed this May 18, 2018
@prybalko prybalko deleted the latest branch May 18, 2018 08:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 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.

None yet

6 participants