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.

Sign up for GitHub

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

Jump to bottom

v3.0: loading: true for first render always, even if all data is available in cache #6659

Closed
nsmith7989 opened this issue Jul 20, 2020 · 9 comments · Fixed by #6710
Closed

v3.0: loading: true for first render always, even if all data is available in cache #6659

nsmith7989 opened this issue Jul 20, 2020 · 9 comments · Fixed by #6710
Assignees
@benjamn
Labels
🐞 bug 🛬 fixed-in-prerelease 🐎 performance
Milestone
Post 3.0

Comments

@nsmith7989
Copy link

nsmith7989 commented Jul 20, 2020 •
edited
Loading

Intended outcome:

Attempting to upgrade to @apollo/client@3.0 but I found that my application was rendering a lot more often than it was in 2.0. We have several components that are hooked into the same query, they usually only render once if the query has already completed earlier.

Actual outcome:

In 3.0 it appears that every query will have loading: true even if the cache data can satisfy the query. It will then immediately re-render with loading: false

How to reproduce the issue:

I recreated the 3.0 issue here: https://codesandbox.io/s/serene-yalow-lvfm9 look at the console and you'll observe the behavior.

Compared to 2.0: https://codesandbox.io/s/determined-galois-e1w2w if you look at the console the same query only renders 1 time with loading: false

Versions

@apollo/client@3.0.2
@nsmith7989
Copy link
Author

This is likely the same underlying issue as #6651, but this is reproducible without a SSR setup.

@MaxHogervorst
Copy link

Same issue without SSR

@benjamn
Copy link
Member

benjamn commented Jul 21, 2020 •
edited
Loading

@nsmith7989 Thanks for the reproduction (it really helps), but I'm not seeing how the data is already available in the cache. I agree we should be able to return a complete loading: false cache result if there is adequate data in the cache. Could you modify the reproduction to do cache.restore(data) using data obtained separately from cache.extract(), so it's actually testing the warm-cache case?

@nsmith7989
Copy link
Author

nsmith7989 commented Jul 21, 2020 •
edited
Loading

@benjamn not completely sure I'm following. In the reproduction above the cache is primed because a parent component ran a query and returned a loading indicator while it was in flight. After parent gets back loading: false it renders children, which happens to run the exact same query as the parent.

My assumption is that the parent component primed the cache and the child is essentially reading from the cache.

In 2.0 the scenario we get is loading: false and data. In 3.0 the first render is loading: true and data from the cache, then a second render where loading: false

@benjamn
Copy link
Member

benjamn commented Jul 21, 2020

@nsmith7989 Ack, sorry, I see how that works now. No need to update the repro. We'll take a look and get back to you soon.

@ZauberNerd ZauberNerd mentioned this issue Jul 23, 2020
Closed
@benjamn benjamn mentioned this issue Jul 23, 2020
Closed
benjamn added a commit that referenced this issue Jul 27, 2020
@benjamn
Move cache-first fallback logic after first network request.
8be3800 
PR #6353 explains the rationale for switching to a cache-first FetchPolicy
after an initial cache-and-network or network-only policy.

When #6365 was implemented, options.fetchPolicy was examined only once, at
the beginning of fetchQueryObservable, so the timing of changing
options.fetchPolicy did not matter as much. However, fixing #6659 involves
checking the current options.fetchPolicy whenever the QueryData class
calls this.currentObservable.getCurrentResult(), so it's now more
important to delay changing options.fetchPolicy until after the first
network request has completed.
benjamn added a commit that referenced this issue Jul 27, 2020
@benjamn
Avoid loading:true results for warm cache reads when possible.
5fe8fee 
Fixes #6659 and #6686.
@benjamn benjamn mentioned this issue Jul 27, 2020
Merged
@benjamn
Copy link
Member

benjamn commented Jul 27, 2020

@nsmith7989 This issue should be fixed in @apollo/client@3.1.0-pre.3, thanks to #6710, if you want to give it a try. Here's an updated version of your reproduction that seems to show the desired loading: false behavior: https://codesandbox.io/s/dazzling-snow-6vvx3

@benjamn benjamn self-assigned this Jul 27, 2020
@benjamn benjamn added this to the Post 3.0 milestone Jul 27, 2020
@nsmith7989
Copy link
Author

nsmith7989 commented Jul 27, 2020 •
edited
Loading

Thanks @benjamn. I can confirm that this looks good in the reproduction.

I will try it on my own app @apollo/client@3.1.0-pre.3 and let you know if I find issues.

@cecchi
Copy link

cecchi commented Jul 27, 2020

We've tested on our own large React codebase and 3.1.0-pre.3 seems to do the trick to fix our SSR-hydration issues. Thanks @benjamn!

@benjamn
Copy link
Member

benjamn commented Jul 28, 2020

Following up: we just published @apollo/client@3.1.0 to npm!

@dan-cooke dan-cooke mentioned this issue Jan 6, 2022
Closed
@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.
Assignees

@benjamn benjamn

Labels
🐞 bug 🛬 fixed-in-prerelease 🐎 performance
Projects
None yet
Milestone
Post 3.0
4 participants
@benjamn @cecchi @nsmith7989 @MaxHogervorst