magento / pwa-studio

🛠Development tools to build, optimize and deploy Progressive Web Applications for Magento 2.
https://developer.adobe.com/commerce/pwa-studio/
Open Software License 3.0
1.07k stars 683 forks source link

[bug]: Category will always show stale data #2622

Closed Jordaneisenburger closed 4 years ago

Jordaneisenburger commented 4 years ago

Describe the bug Categories always shown stale data. This happens because the useLazyQuery in category.js doesn't have a fetch policy. A quick solution could be setting fetchPolicy: 'cache-and-network' but I found that this strategy doesn't actually do what you'd expect it to do.

What I thought this would do is: return data from cache immediately and in the background do a call to prevent stale data.

What it actually seems to do: Basically do the same as network-first. If you cache 2 categories and flush magento cache (making the first request slow) you will see it won't render the category (that we already have in client side cache) until the request is finished. When the request is inflight even tho we already have it in cache the loading prop will become true making it look janky.

The solution build for the product page in useProduct.js seems to be doing exactly what we'd expect. I Think we need this same functionality for the category pages.

I tried building useCategory.js but it seems like we cache all the data separately so we can't simply open the cache and match a fragment?

To reproduce Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Expected behavior A clear and concise description of what you expected to happen.

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem here.

Possible solutions Add any ideas about possible solutions to the problem here.

Please complete the following device information:

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

m2-assistant[bot] commented 4 years ago

Hi @Jordaneisenburger. Thank you for your report. To help us process this issue please make sure that you provided sufficient information.

Please, add a comment to assign the issue: @magento I am working on this


Jordaneisenburger commented 4 years ago

So I managed to get something working the same way we would do for product pages but not sure if this makes sense or if there's a better way/flow.

// category.js
const { runQuery, talonProps } = useCategory(GET_CATEGORY);
const { loading, error, data } = talonProps;
// useCategory.js
import { useMemo, useCallback, useState, useEffect } from 'react';
import { useApolloClient, useLazyQuery } from '@apollo/react-hooks';

export const useCategory = query => {
    const [stateVariables, setStateVariables] = useState();

    const runQuery = useCallback(props => {
        setStateVariables(props);
    }, []);

    const apolloClient = useApolloClient();

    const categoryFromCache = useMemo(() => {
        if (!stateVariables) return null;
        try {
            return apolloClient.readQuery({
                query: query,
                variables: { ...stateVariables.variables },
            });
        } catch (e) {
            // The category is in the cache but it is missing some fields the fragment needs.
            return null;
        }
    }, [apolloClient, stateVariables, query]);

    /*
     * Fetch the category from the network.
     *
     * Note that this always fires, regardless of whether we have a cached category or not:
     * If we do not have a cached category, we need to go fetch it from the network.
     * If we do have a cached category, we want to ensure its cache entry doesn't get stale.
     * In both cases, Apollo will update the cache with the latest data when this returns.
     */
    const [networkQuery, queryResults] = useLazyQuery(query, {
        fetchPolicy: 'network-only',
    });

    const { error, loading, data: categoryFromNetwork } = queryResults;

    useEffect(() => {
        if (stateVariables) {
            networkQuery({
                variables: { ...stateVariables.variables },
            });
        }
    }, [networkQuery, stateVariables]);

    const category = useMemo(() => {
        if (categoryFromCache) {
            return categoryFromCache;
        }

        if (categoryFromNetwork) {
            return categoryFromNetwork;
        }

        // The category isn't in the cache and we don't have a response from GraphQL yet.
        return null;
    }, [categoryFromNetwork, categoryFromCache]);

    return {
        runQuery,
        talonProps: {
            error,
            loading: categoryFromCache ? false : loading,
            data: category,
        },
    };
};
sirugh commented 4 years ago

cache-and-network should behave like you'd expect - it should return cached data immediately and true-up with the network. I think this might actually just be as simple as only rendering the loading state when loading && !data.

If that doesn't work could you provide a video of repro? I really want to avoid more manual implementations of cache-and-network 😄

Jordaneisenburger commented 4 years ago

I'm such a noob haha :/ I somewhere knew I was doing something stupid. Thanks Stephan!

brendanfalkowski commented 4 years ago

Been working on PWA Studio for a long time, and still have pretty low confidence I've seen all the Apollo / cache / network / loading && !data edge cases and understand the techniques and reasons for solving each.

We had so many issues with GQL caching, that we just switched everything to network-only for development to revisit later and... now it's later. Almost all of our render flows are like this because it's dead simple to read:

if (loading) {
    return <Loading />;
}

if (error) {
    return <p>Error loading X info.</p>;
}

return <div>the data</div>;

It would be great to see a DevDocs page documenting the preferred cases for each fetch-policy and the knock-on effect to the destructured data, error, loading and render flow logic as cache responds or updates.

Here: https://magento.github.io/pwa-studio/technologies/basic-concepts/client-side-caching/

The last section could be its own page with physical examples.

cc @jcalcaben

sirugh commented 4 years ago

@Jordaneisenburger I'm going to reopen this issue because it does warrant a fix, and it should be really easy - just applying cache-and-network fetch policy will do the trick.

sirugh commented 4 years ago

We had so many issues with GQL caching

@brendanfalkowski that's unfortunate! I'd be happy to listen to the pain points if you'd like to share in another issue or on slack. For what it's worth, apollo client has been an evolving project and I'm in the middle of upgrading us to v3 which should make things a bit easier to understand. I'd also be happy to work with @jcalcaben on documentation. Let me know how I can help!

jcalcaben commented 4 years ago

@sirugh @brendanfalkowski Yes! Let's brainstorm what you would like to see in that doc topic, such as what use cases/issues you've encountered and which solutions worked for you.

brendanfalkowski commented 4 years ago

@sirugh — Partly, this is because we forced multistore and B2B account switching into the project. The Apollo cache wouldn't differentiate the same requests in different stores/accounts until we figured out why. The short-term fix was just "yay no cache". It also became frustrating when working on the GQL API and the frontend using it at the same time, cache kept giving stale responses so we stopped using it altogether (for dev) over policies like cache-and-network.

@jcalcaben To avoid flooding this issue, I made a new one here: https://github.com/magento/pwa-studio/issues/2635

awilcoxa commented 4 years ago

Created PWA-862

tjwiebell commented 4 years ago

Fixed in #2686