[bug]: Home Page query is not fetching from network/updating cache

[luke-denton-aligent]

Describe the bug
I've noticed that Apollo in PWA Studio isn't updating its local cache (apollo-cache-persist in localStorage) from the network (for page content in my case), it's only serving from cache. This means that making changes to a page doesn't ever get reflected in the app until this cache is manually cleared.

To reproduce
Steps to reproduce the behavior:

1. Navigate to homepage in app (allowing content to be cached)
2. Open Admin
3. Go to Content > Pages > Home page
4. Edit the home page and save
5. Refresh the application (the old content will display, which is expected for the first reload)
6. Refresh the application again
7. The old content will be displayed again, as it will for any number of times that the page is refreshed
8. Manually clear the localStorage property, apollo-cache-persist
9. Refresh the application
10. Now the updated home page data will be displayed

Expected behavior
On the first reload, the old data should be displayed, but in the background a request should be sent to the server to get the latest version of the homepage. The returned data should then being pushed into the localStorage cache, so that on the second reload (either a user is prompted through an action similar to "There is a new version of the app available, please refresh", or they just happen to reload again), they will see the new data on the page

Possible solutions
The solution is to make a network request to update Apollo cache after it has server up its cache. I'm not familiar enough with Apollo to say how this would be done specifically

Please complete the following device information:

• Device [e.g. iPhone6, PC, Mac, Pixel3]: PC
• OS [e.g. iOS8.1, Windows 10]: Centos 7
• Browser [e.g. Chrome, Safari]: Chrome
• Browser Version [e.g. 22]: 80.0.3987.132
• Magento Version: 2.3.3
• PWA Studio Version: 5.0.1

Please let us know what packages this bug is in regards to:

• venia-concept
• venia-ui

[brendanfalkowski]

During development, we started passing {fetchPolicy: 'network-only'} in every GQL request because this became too annoying for every B2B context (multi-site, multi-account, multi-user).

Docs for this do not exist: apollographql/apollo-client#1626

But somebody blogged it: https://medium.com/@galen.corey/understanding-apollo-fetch-policies-705b5ad71980

We have a pre-launch ticket to review all these, and consider where cache-and-network or other configs make sense for performance.

[sirugh]

On the first reload, the old data should be displayed, but in the background a request should be sent to the server to get the latest version of the homepage.

This is how cache-and-network works, as @brendanfalkowski said. When we originally coded the gql queries we did not fully utilize the fetch policies so you may see some behavior like this across the app.

The returned data should then being pushed into the localStorage cache, so that on the second reload (either a user is prompted through an action similar to "There is a new version of the app available, please refresh", or they just happen to reload again), they will see the new data on the page

Actually, if we code the components correctly we won't have to use the service worker for this (that's what displays the "new version available" toast), we can just trust that Apollo will "show" the correct thing, as soon as it can. In this case it will render cached data first, make a network request, and then re-render with any changed data.

Let's modify the title/contents of this ticket to be the specific behavior you noticed, ie "Home Page query is not fetching from network/updating cache", etc. And then @awilcoxa and @Jordaneisenburger can help get it prioritized.

[awilcoxa]

Moved to Jira backlog for prioritization in grooming

[awilcoxa]

Marked as P2S1, needs additional investigation

[davemacaulay]

I've opened a draft PR which implements the cache-and-network fetch policy for the CMS Page here: #2453.

@sirugh @jimbo there are a number of "bugs" or "issues" with how Apollo handles the loading & networkStatus state when using the cache-and-network fetch policy. It'll leave loading as true even if it manages to hit the cache for the data. Meaning the user would have to wait for the full network request even though the data was fetched from the in-memory cache.

There's a lot of closed & open issues on Apollo for this, such as apollographql/react-apollo#3361, there is also a comment from one of the contributors which states this is actually an expected outcome, and thus would need to be resolved by a feature request: apollographql/react-apollo#2739 (comment)

The solution I found, which works in my initial testing is to expand the if (loading) check to also validate if the expected data is present, this results in the following:

    if (loading && (!data && !data.cmsPage)) {
        return fullPageLoadingIndicator;
    }

From my testing this works for completely uncached requests, cached requests with no data changes from Magento and finally cached requests with changes in Magento. You'll see the original cached data render, then after the GraphQL requests completes the new content will be automatically rendered.

I couldn't find a concise opinion on how to handle this from my research so wanted to hear your thoughts on whether this is an adequate solution.

[jimbo]

loading

@sirugh @jimbo there are a number of "bugs" or "issues" with how Apollo handles the loading & networkStatus state when using the cache-and-network fetch policy. It'll leave loading as true even if it manages to hit the cache for the data. Meaning the user would have to wait for the full network request even though the data was fetched from the in-memory cache.

There's a lot of closed & open issues on Apollo for this, such as apollographql/react-apollo#3361, there is also a comment from one of the contributors which states this is actually an expected outcome, and thus would need to be resolved by a feature request: apollographql/react-apollo#2739 (comment)

Right. From Apollo's docs:

A boolean representing whether or not a query request is currently in flight for this component. This means that a query request has been sent using your network interface, and we have not yet gotten a response back. Use this property to render a loading component.

However, just because data.loading is true it does not mean that you won’t have data. For instance, if you already have data.todos, but you want to get the latest todos from your API data.loading might be true, but you will still have the todos from your previous request.

There are multiple different network states that your query may be in. If you want to see what the network state of your component is in more detail then refer to data.networkStatus.

Solutions

The solution I found, which works in my initial testing is to expand the if (loading) check to also validate if the expected data is present, this results in the following:

    if (loading && (!data && !data.cmsPage)) {
        return fullPageLoadingIndicator;
    }

From my testing this works for completely uncached requests, cached requests with no data changes from Magento and finally cached requests with changes in Magento. You'll see the original cached data render, then after the GraphQL requests completes the new content will be automatically rendered.

I couldn't find a concise opinion on how to handle this from my research so wanted to hear your thoughts on whether this is an adequate solution.

It looks to me like the docs recommend using networkStatus, which we do in some other places where we have this issue. Basically, whenever loading is ambiguous because of caching policy, we should look at networkStatus—and that should probably be a lot more often as we optimize our caching.

That said, as long as this conditional (loading && !data) covers all of the use cases, I'm fine with it. 👍

[jordan-cutler]

@jimbo testing locally, it looks like network status is solely 1 (the normal loading state) or 7 (ready). Therefore, you wouldn't get any more info from using networkStatus instead of loading.

It seems like the approach @davemacaulay came up with is the best and only option, unless if Apollo were to incorporate this use case into the API