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

Set loading to false if data is already in cache #5971

Closed
wants to merge 1 commit into from

Conversation

vishwam
Copy link
Contributor

@vishwam vishwam commented Feb 20, 2020

This is a work-in-progress PR to fix #5869 (and possibly #5835 and #5947): useQuery currently returns loading: true even if the data is already in the cache (and then re-renders with loading: false, causing a flash with the loading UI.)

I narrowed it down to ObservableQuery.getCurrentResult - it gets the data and partial from the QueryManager (which gets it from the cache). If we know we already have the complete data, I think we can short-circuit the queryStore.get() flow and return result with loading: false. I've tested this locally with various scenarios, and am able to get back to v2's behavior (only one render with loading: false).

Please let me know if this is the correct approach. If it is, I can fix the tests and send an update to this PR (it's failing because currentResult no longer matches observableQuery.next - I'd also like some pointers on how best to fix this.)

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

@vishwam vishwam requested a review from benjamn February 20, 2020 06:18
@benjamn
Copy link
Member

benjamn commented Feb 20, 2020

At the very least this logic needs to take the fetchPolicy into account. I can only imagine this being appropriate for the cache-first fetch policy, where the presence of complete cache data means there won't be a network request. For policies like cache-and-network, getting data from the cache definitely does not mean the request is finished!

As a side note, this change triggered a lot of very much related test failures. If you're going to make a small change that has big consequences, at the very least you need to spend some time really understanding why those tests are failing. Just making the most obvious changes to get them to pass is not going to work for us.

@vishwam vishwam closed this Feb 21, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 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.

[3.0.0-beta.27] Querying data that is already in the cache sets loading to true
2 participants