liteflow-labs / starter-kit

NFT Marketplace running on the Liteflow infrastructure
https://demo.liteflow.com
Apache License 2.0
11 stars 43 forks source link

Deactivate Apollo client cache #63

Closed NicolasMahe closed 1 year ago

NicolasMahe commented 2 years ago

I would like to open the discussion about completely removing the cache of Apollo.

Currently, the codebase doesn't properly implement the cache system of Apollo.

Only type Account is setup to merge data with the same address and I'm not even sure why.

Apollo seems to cache by default lazy query (bug because of cache on lazy query: https://github.com/liteflow-labs/nft/pull/1948) but not on all version.

My suggestion is to deactivate the cache of Apollo for now:

return new ApolloClient({
    link: authLink.concat(httpLink),
    cache: new InMemoryCache({
      typePolicies: {
        Account: {
          keyFields: ['address'],
        },
      },
    }),
    ssrMode,
+    defaultOptions: {
+      query: {
+        fetchPolicy: 'no-cache',
+      },
+      mutate: {
+        fetchPolicy: 'no-cache',
+      },
+      watchQuery: {
+        fetchPolicy: 'no-cache',
+      },
    },
  })

https://github.com/liteflow-labs/nft/blob/4ebf01b1a77741542576296efdbd7e70a0081f6f/packages/components/src/client.ts#L51-L61

This will help remove any side-effect caused by un-wanting cache.

We can always activate the cache per request if we need to.

@liteflow-labs/nft-marketplace I would like to have your feedback on this one.

cpvdeveloper commented 2 years ago

In general, I like the idea of deactivating the cache. I've deactivated it on multiple projects in the past, not just with Apollo but also with URQL.

I've found that caching usually causes more problems that it solves, which seems to be the case here too. As you say, we could override the behaviour on a per-request basis, but again from my experience, even this is rarely necessary.

antho1404 commented 2 years ago

yep 100% agree on that 👍

cpvdeveloper commented 1 year ago

I have realised that this would require a big refactor. From my understanding, we rely on the cache on nearly every page:

  1. We fetch some data on the server (in getServerSideProps) using client.query(...)
  2. We fetch the exact same data on the client using the corresponding Apollo hook. Exact same meaning same query and same variables
  3. Because the same query has already been run on the server, the fetch on the client-side (using the hook) has data on the first render and therefore has no initial loading state

If we disabled the cache, this 3rd point would no longer be true, and we'd either need to handle an initial loading state, OR, pass the initial data as props from getServerSideProps.

EmmanuelDrouin commented 1 year ago

@antho1404 could you elaborate on the "effort" required for this? Chris is mentioning a "big refactor" I would like to understand the effort on our side to take a decision on this issue ;)

antho1404 commented 1 year ago

This still needs more research to see if it even makes sense to remove the cache because of the SSR. For information, I have already started to work on some improvements on the SSR and the apollo client/cache here https://github.com/liteflow-labs/starter-kit/pull/118. This will give more answers

antho1404 commented 1 year ago

fixed by https://github.com/liteflow-labs/starter-kit/pull/300