apollographql / apollo-feature-requests

🧑‍🚀 Apollo Client Feature Requests | (no 🐛 please).
Other
130 stars 7 forks source link

Cache redirects don't work with interfaces specified by possibleTypes #382

Open bluepichu opened 1 year ago

bluepichu commented 1 year ago

Issue Description

I asked this question on the Discord server and got no response. I think this is a bug, but it's possible I'm misunderstanding if/how these two features are supposed to interact.

I have a schema that looks like this:

type Query {
    widgets: [Widget!]!
    widget(id: ID!): Widget
}

interface Widget {
    id: ID!
    name: String!
}

type FooWidget implements Widget {
    id: ID!
    name: String!
    foo: String!
}

type BarWidget implements Widget {
    id: ID!
    name: String!
    bar: String!
}

I have a page where I'm fetching a full list of available widgets via query { widgets { id name } }, and then rendering the list of widgets. However, the "widget card" component used within that list is written in such a way that it can be reused in multiple locations, possibly including some where we don't already have the relevant data, and therefore is in charge of fetching its own data via query($id: ID!) { widget(id: $id) { id name } }. Naturally, this means that we end up round-tripping once for the list and then again for each list item.

The documented solution to this is to use a cache redirect, so I've set up my cache like this:

const apolloCache = new InMemoryCache({
    typePolicies: {
        Query: {
            fields: {
                widget: {
                    read(_, { args, toReference }) {
                        return toReference({
                            __typename: "Widget",
                            id: args.id
                        });
                    }
                }
            }
        }
    },
    possibleTypes: {
        Widget: ["FooWidget", "BarWidget"]
    }
});

My understanding is that this should allow the individual cards to fetch their data directly from the cache since all of the data they're requesting is already being loaded as part of the list query. However, I still see a second network request for each card going through. Stepping through the code in the developer tools, I noticed that when id is abc, then Apollo is reaching an error state because there is no value in the cache with key Widget:abc (in particular, the message Dangling reference to missing Widget:abc object is being propagated up to ObservableQuery.getCurrentResult). This is because the normalized key is FooWidget:abc since that's the actual concrete type of the widget in question. It seems like the possibleTypes configuration is not being respected with regards to checking for that key in the cache.

Link to Reproduction

https://github.com/bluepichu/react-apollo-cache-issue

Reproduction Steps

  1. Load the page
  2. Observe that the person list immediately jumps from the base "loading" state to a complete list, while the widget list goes through a step in the middle where the individual items are loading
  3. Open the logs
  4. Observe that the people data (which doesn't use interfaces at all) was only loaded once, while the widget data (which does use interfaces with possibleTypes) was loaded both as a complete list and as the individual items, despite the fact that the list query should provide sufficient information for the individual item queries to use the cache
bignimbus commented 1 year ago

Hi @bluepichu 👋🏻 thanks for opening this issue and thanks for your patience. First, it's totally understandable that a user would expect possibleTypes to factor into a toReference so thanks for this feedback. I believe that updating the default behavior would be a breaking change so I don't see that happening in version 3.x. That said, this is an interesting idea and something we want to explore further 🙏🏻

bluepichu commented 1 year ago

Thanks for getting back to me!

I think this is definitely worth looking into in the future since the expectation that toReference would work with inheritance is fairly reasonable, but in the meantime I've found a suitable workaround: using the keyFields type policy to customize the cache id. In my example repo, I just made this diff:

diff --git a/src/index.jsx b/src/index.jsx
index afc7a5f..9a2e416 100644
--- a/src/index.jsx
+++ b/src/index.jsx
@@ -127,6 +127,9 @@ const client = new ApolloClient({
             }
           }
         }
+      },
+      Widget: {
+        keyFields: (object) => `Widget:${object.id}`
       }
     },
     possibleTypes: {

Due to the typePolicy inheritance enabled by possibleTypes, FooWidget and BarWidget inherit this keyFields configuration and have cache keys of the form Widget:<id> instead of FooWidget:<id> or BarWidget:<id>. This makes the cache lookups resolve properly, and in my case is sufficient because it's guaranteed that widgets (even those of different types) will never have conflicting ids.

That said, it wasn't particularly obvious from the documentation or the name of the field that you could do this with keyFields; the docs made it sound like this behavior could only be accomplished via dataIdFromObject. I only figured this out because I was poking around in the TS types while checking the type of something unrelated. It might be good to add an example of using a function to fully specify the cache key in this section of the documentation, and might even be worth considering changing the name of the field in a future version since in this case the function generates the cache key directly rather than having anything in particular to do with fields of the object as the name suggests.