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

Add checking for existence of fetchMoreQuery in store #3367

Merged
merged 7 commits into from
May 25, 2018
Merged

Add checking for existence of fetchMoreQuery in store #3367

merged 7 commits into from
May 25, 2018

Conversation

conrad-vanl
Copy link
Contributor

@conrad-vanl conrad-vanl commented Apr 24, 2018

Fixes #3345

We are noticing that there are scenarios where this.store[fetchMoreForQueryId] is undefined, which throws an error and causes an RSOD in react-native. I am having issue reliably recreating this issue, thus I did not add test coverage for this yet - but happy to do so if we feel it's needed for this case.

I believe the logic added in this PR results in safer code, but I'm not sure if the fact that store not containing fetchMoreForQueryId points to another issue elsewhere. It seems logical to include the conditional check that's added in this PR, especially since we check for this.store[queryId] just a few lines above, but wanted to call that out.

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.
  • Fix current tests
  • Add changelog

@conrad-vanl conrad-vanl changed the title Add checking for existence of fetchMoreQuery in store Add checking for existence of fetchMoreQuery in store Apr 24, 2018
@apollo-cla
Copy link

apollo-cla commented Apr 24, 2018

Warnings
⚠️

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

Generated by 🚫 dangerJS

@rtymchyk
Copy link

It does seem to point to a larger issue, but given that I have no way of even catching this error, it would be good to have this guard in place. It's by far the most frequent Apollo error we face in our app.

@Jarred-Sumner
Copy link

Jarred-Sumner commented May 15, 2018

Any update on this? This bug causes frequent crashes in React Native apps when infinite scroll is used on FlatList/SectionList and that FlatList/SectionList gets unmounted

@conrad-vanl
Copy link
Contributor Author

As far as I can tell this PR fixes the error and crash on react native, just needs some 👀 from a maintainer

@rtymchyk
Copy link

@helfer maybe some 👀 on this? It's just a 2 liner guard :) Happy to help push this forward where needed

@hwillson hwillson self-assigned this May 24, 2018
@hwillson
Copy link
Member

@rtymchyk I'll have this reviewed today. We should be able to get this in place for our next release (May 29th).

@rtymchyk
Copy link

Thank you very much!

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.

Thanks for this PR @conrad-vanl! I've added a few tests to verify things, and everything looks great. We should be all set to get this merged. Thanks again!

@hwillson hwillson merged commit b4ff8a4 into apollographql:master May 25, 2018
@hwillson hwillson mentioned this pull request May 25, 2018
3 tasks
@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.

Potential issue when this.store[fetchMoreForQueryId] is undefined
5 participants