Remove experimental cache.modify method.

[benjamn]

Note: this PR has been reverted, with important additional improvements, in #6350! 🎢 ✨

Older note: please read #6289 (comment) (below) for additional explanation of this removal!

The cache.modify API was first introduced in #5909 as a quick way to transform the values of specific existing fields in the cache. At the time, cache.modify seemed promising as an alternative to the readQuery-transform-writeQuery pattern, but feedback has been mixed, most importantly from our developer experience team, who have helped me understand why cache.modify would be difficult to learn and to teach.

While my refactoring in #6221 addressed concerns about broadcasting cache.modify updates automatically, the bigger problem with cache.modify is simply that it requires knowledge of the internal workings of the cache, making it nearly impossible to explain to developers who are not already Apollo Client 3 experts. As much as I wanted cache.modify to be a selling point for Apollo Client 3, it simply wasn't safe to use without a firm understanding of concepts like cache normalization, references, field identity (keyArgs), read/merge functions and their options API, and the options.toReference(object, true) helper function (#5970).

By contrast, the readQuery-transform-writeQuery pattern may be a bit more verbose, but it has none of these problems, because these older methods work in terms of GraphQL result objects, rather than exposing the internal data format of the EntityStore.

Since cache.modify was motivated to some extent by vague power-user performance concerns, it's worth noting that we recently made writeQuery and writeFragment even more efficient when rewriting unchanged results (#6274), so whatever performance gap there might have been between cache.modify and readQuery/writeQuery should now be even less noticeable.

One final reason that cache.modify seemed like a good idea was that custom merge functions can interfere with certain writeQuery patterns, such as deleting an item from a paginated list. If you run into trouble with merge functions running when you don't want them to, we recommend calling cache.evict before cache.writeQuery to ensure your merge function won't be confused by existing field data when you write your modified data back into the cache.

[benjamn]

Those test failures look legitimate, but I'll investigate next week.

[hwillson]

These changes are 👍 @benjamn - thanks!

[darkbasic]

@benjamn let me say that I strongly disagree with that (and it's probably an understatement). Removing this functionality is a HUGE stepback IMO and was one of the main reasons why I was interested in Apollo 3 in the first place.

it simply wasn't safe to use without a firm understanding of concepts like cache normalization

If you don't know how and why Apollo does normalization you shouldn't use it in the first place, because you're basically a time bomb ready to do damage.

references

What's so difficult about references? Some of the best trending libraries use them (see https://mikro-orm.io/docs/entity-references/) and their users are super-happy about it: there is even discussion in the state tracker about how to further improve it and it's already happening in the dev branch.
Did I like the Apollo 3 ref system? No, IMO there were several things to improve. First of all a typescript helper to ref-erize types was really needed, but improvements could be made without having to throw everything away.

field identity (keyArgs)

That's the whole point of cache.modify. Nobody forces you to use it and if some corporation is against it we could have simply created a linting rule to forbid it. Why should everyone else be forced to not use it?

Also, let me say that I have a whole application written with Apollo 3 almost ready to go to production thanks to this feature alone. I know this is not a stable branch, but the benefits far exceeded the few bleeding edges and it was never stated before that cache.modify was an experimental feature: an RC was even expected in May!

How am I supposed to update paginated queries with dozens of filters (including search ones) with readQuery and readFragment whenever I delete an item? Should I perform a writeQuery on each and every possible combination of args, including search terms!?

Should I use cache.evict and just refetch all of them for no reason at all?

Really, I don't understand: Apollo 3 was shaping up so well :(

I see a "discussion" label on this, but you can't really expect to drop a bomb like this just before the weekend and expect everyone else to notice in time.

[ivan-kleshnin]

Bad news. We also really waited for something to improve in this area.

We never use readQuery and writeQuery at the team because they couple every freaking part of the system together. XModel has to know everything about YModel and ZModel and so on.
It's almost a definition of bad code and bad architecture.

So the only tool at our disposal is neandertal resetStore() 😢

Having said that, I appreciate the amount of work done by @benjamn and I trust his intuition.
Maybe we need to step back one more time and find another angle to attack the problem.

[3nvi]

I have to agree. Removing it close to the release date (according to Apollo's Twitter), after more than 3 months is a big setback. As a company, we now have to dedicate time to refactor code back to what it already was. As mentioned in a similar comment above, it was the main reason we switched from Apollo v2 to Apollo v3. I don't understand why supporting it creates a problem, since the previous readQuery and writeQuery still work fine.

I understand that beta testers will face breaking changes, but a removal of such a big feature was not expected from my side; especially so late down the "beta".

[hwillson]

Thanks for the really great feedback all. We're discussing this further internally and will post back shortly.

[benjamn]

First of all, thanks to everyone for using (really using!) the betas, for your patience while we address feedback about bugs and usability from lots of different developers with different needs and appetites for different kinds of complexity, and for paying enough attention to have strong feelings about this removal. We deeply appreciate your help.

I am open to reconsidering how the cache.modify API could/should work, and I invite you all to participate in the redesign. However, I do not see any scenario where this PR simply gets reverted, so please don't push for that (it will be a waste of time).

As a more detailed example of the kind of mistakes that I am sure people would make using cache.modify, suppose you need to append a new object to the end of an existing list:

cache.modify({
  todos(existing) {
    return [...existing, newTodo];
  },
}, cache.identify({
  __typename: "TodoList",
  id: "...",
})

Unfortunately, this code is broken (assuming the existing elements are normalized references, which is pretty likely, though not always the case), because you'll be appending a non-reference (newTodo) to a list of { __ref: ... } objects, and the old version of cache.modify would totally just let you mix the types like that.

If you realized that you need to use options.toReference to turn your data into a { __ref: ... } object before appending it, congratulations, that already makes you an uncommonly diligent, defensive programmer:

cache.modify({
  todos(existing, { toReference }) {
    return [...existing, toReference(newTodo)];
  },
}, cache.identify({
  __typename: "TodoList",
  id: "...",
})

But this code is still broken! The todos list will now be updated appropriately, but the toReference(newTodo) references may not actually point to anything in the cache. This is because toReference only creates a reference, without modifying/creating the cache data that the reference points to. That behavior is useful in cases where you want to redirect to data that may or may not exist elsewhere in the cache, but it's the wrong behavior here, leading to the accidental loss of fields like (say) newTodo.description and newTodo.completed.

The full solution is to let toReference(newTodo) update the cache with the fields you're providing, using toReference(newTodo, true), an obscure but important feature that we added in #5970. However, you'd better hope all those new fields are primitive scalars, without any arguments, or you'll have to recursively process the contents of newTodo by hand, guessing a lot of implementation details along the way, probably calling into the Policies class yourself to make sure you're respecting all keyFields and keyArgs configurations.

You could get it to work, but readQuery and writeQuery do all this processing and updating automatically, which is why I am much more comfortable asking developers to put up with the boilerplate of that approach (which does suck, I know) than asking them to learn all the details I described above. And please remember the subtleties of toReference are just one piece of a non-trivial puzzle of essential knowledge.

Even if you were nodding along to everything I said above (feel free to let me know if that's the case, so I know there's at least a few of you out there, hi 👋 ), I sincerely do not believe you can expect all of your teammates to internalize these details before interacting with cache.modify code in your codebase. You'd have to be working alone, highly confident in your own understanding of the InMemoryCache and EntityStore, to make it work.

I'll post a follow-up comment with some ideas about how cache.modify could work differently, but I hope this explanation was helpful for getting everyone on the same page about my/our reasoning for removing the current implementation.

[angelnar87]

@benjamn There is a few of us out here that nodded along ;)
cache.modify may not be the right solution for everyone, but it's definitely "the only" reason some of us migrated to Apollo Client v3. I am more likely to stay in beta43 than go back to the coupled code offered by readQuery/writeQuery. That would be a huge setback :(

[darkbasic]

Those are all valid points and yes, we were aware of it. I may be speaking for myself, but I'm pretty sure that most of us beta-testers are going through the various PRs and I have to say that you always do a wonderful job writing extensive descriptions detailing the internals.
I don't expect everyone to fully understand the innings of the store, but I still prefer to have the tools to get the job done: readQuery/writeQuery is not really a viable solution for the use case I described before. It isn't just a matter of being more verbose/writing more boilerplate.
Chosing to opt out is really easy because you just have to write a linting rule for that, but I guess that if we manage to engineer a better solution it's a win-win.

[jedwards1211]

I haven't tried the beta yet, but I've been more excited about the typePolicies API, which will replace a lot of the cases where I was using writeQuery/writeFragment. So I'm curious to see examples of what people want to use cache.modify for, and whether typePolicies might actually be a more elegant solution for some of those cases?

[stemmlerjs]

First of all, I want to thank everyone who has tried the new beta. It's great to see your feedback here. I also really want to thank @benjamn for all of his hard work on this. I fully agree with your comments. As a newcomer to Apollo Client, I went the cache.modify route and made all of the mistakes you described, even as someone who pretty much had direct access to the client team every step of the way 😂.

Out of empathy for the new Apollo Client user, I think the decision to remove cache.modify makes sense. To developers familiar with the essential internal knowledge of that API, perhaps like @jedwards1211, I have yet to witness a code example of something that was more clear with cache.modify than with readQuery/writeQuery and that was impossible with readQuery/writeQuery and only possible with cache.modify.

To better understand where people fall off, I think the learning path for newcomers to Apollo Client looks something like this:

1. Basics of Apollo Client (what it is, how it works)
2. Learn how to use useQuery, and useMutation to get and set data.
3. Run into a situation where the UI didn't update, then either:

• (preferred, but less common) discover readQuery/writeQuery and how to use the update function to update the UI when working with lists
• (not preferred, but more common) use refetchQueries to fix their problem

Regardless of how users accomplish (3), I think these three learning milestones empower beginners to accomplish about 95% of the most common use cases, even with minimal (or no) understanding of how the cache works under the hood.

Here are some of the most common use cases via @apollographql/ac3-state-management-examples.

For example, when deleting a todo item from a list, users can lean on a similar conceptual model they might have from using Redux or React Context reducers to update a list:

const [mutate, { data, error }] = useMutation<
    DeleteTodoTypes.DeleteTodo, 
    DeleteTodoTypes.DeleteTodoVariables
  >(
    DELETE_TODO,
    {
      update (cache, { data }) {
        const deletedTodoId = data?.deleteTodo.todo?.id;
        const allTodos = cache.readQuery<GetAllTodos>({
          query: GET_ALL_TODOS
        });

        cache.writeQuery({
          query: GET_ALL_TODOS,
          data: {
            todos: {
              edges: allTodos?.todos.edges.filter((t) => t?.node.id !== deletedTodoId)
            },
          },
        });
      }
    }
  )

mutate({ variables: { id: 2 }})

This is what you might expect someone just starting with AC to do. This is also not entirely correct. In order to fully delete that todo, we'd need to use cache.evict and do cache.evict(`Todo:${deletedTodoId}`). But unless someone has an understanding of how cache normalization works, it's not reasonable to expect them to do that because it's an internal cache detail.

The same example, but this time using the advanced cache manipulation APIs could look like this:

const [mutate, { data, error }] = useMutation<
    DeleteTodoTypes.DeleteTodo, 
    DeleteTodoTypes.DeleteTodoVariables
  >(
    DELETE_TODO,
    {
      update (cache, el) {
        const deletedId = el.data?.deleteTodo.todo?.id
        
        cache.modify({
          todos (existingTodos, { readField }) { // a
        
            const newTodos = { // b
              ...existingTodos,
              edges: existingTodos.edges.filter((edge: any) => {
                return deletedId !== readField('id', edge.node); // c
              })
            }
            
            return newTodos; 
          }
        });

        cache.evict(`Todo:${deletedId}`) // d
      }
    }
  )

mutate({ variables: { id: 2 }})

Here's the prerequisite knowledge you need to have in order to understand this:

• a: that your queries are normalized in the cache and preserve the order of the responses by maintaining a reference to each of the flattened objects
• a: that existingTodos are references, not actually the todos themselves
• b: that you have to return a reference, not the actual data value (much different than the previous example)
• c: that you can only get the value by tracing the reference using readField.
• d: that each item is split and normalized using by default, your __typename + id field.

I don't think it's fair to assume most people know this without adequate education. So then to play with the big kid tools, my opinion is that users would then need:

4. To learn how cache normalization works (the normalization algorithm, what references are, and what cached collections are)
5. Then, how to use advanced cache manipulation APIs that surgically operate against the cache (cache.modify, cache.evict, etc).

There's a bunch of things we could do here:

• A: Make cache normalization education a more prevalent part of the learning experience, which would solve the education gap around when it's necessary to use cache.evict regardless of if we use cache.modify or cache.writeQuery. I'm currently working on "Demystifying Cache Normalization".
• B: Ditch cache.modify and never look back.
• C: Design some revised version of cache.modify that we're happy with and promote advanced users to use it for those advanced use cases only possible with cache.modify (I would like to see what these are - please share code examples ☺️).

cc @darkbasic

[dmitrybirin]

Hello 👋
Just to bring some additional thoughts of that.

First of all, thanks you, Apollo team for hard work on building great product and being there for devs on GitHub, Twitch sessions etc. Sometimes, product going through some hard decisions. And it’s understandable.

Unfortunately, I’m one of the “along nodders”, that @angelnar87 mentioned :) My team and me are using beta in our new feature for the upcoming release, and using the cache.modify a lot. It’s fantastic API, that allows you to update a lot of particular fields in particular situation with particular logic.
Yes, it was a lot to understand how the API works, and you need to understand how the cache structured. But then everything works perfectly, and going through understanding it - worth it.
That API actually was one of the major selling points to use AC3 in beta for my team.
I wanted to help with docs for cache.modify and makeVar (that looks like a secret API, actually) after we finish our release. Sorry for not doing it sooner.
I don’t know a lot of inner machinery of Apollo team, but for me it looks more like documentation/education/marketing problem, that could be solved.

[WIStudent]

I am kinda sad seeing cache.modify being removed. To me it seemed to be an elegant solution when I needed to remove an entry from parametrized queries.

type Entry {
  id: String!
}

type Query {
  entries (search: String!): [Entry!]!
}

type Mutation {
  removeEntry (id: String!): String!
}

async function removeEntry (variables: RemoveEntryVariables, client: ApolloClient<NormalizedCacheObject>): Promise<void> {
  await client.mutate<RemoveEntry, RemoveEntryVariables>({
    mutation: removeEntryMutation,
    variables,
    optimisticResponse: {
      removeEntry: variables.id
    },
    update (cache) {
      cache.modify({
        entries(value: Reference[], {readField}) {
          return value.filter(entry => readField('id', entry) !== variables.id)
        },
      });
      cache.gc();
    }
  })
}

(full example project can be found here)
Using cache.modify allowed me to remove an Entry from all cached entries queries regardless with what variables they were called.

To achive the same thing using cache.readQuery and cache.writeQuery I need to know the variables that were used with the entries query. So far I haven't found a way to get the used variables from the cache directly, leaving no other option than saving the variables elsewhere manually when triggering the query.

[jrnail23]

Yeah, this one was a big surprise for me when I updated to beta 50.
I really thought that "RC coming in May" meant that the API or at least the feature set would be relatively stable.
In any case, put me down as a vote in favor of either renaming modify to dangerouslyModify (or something else that would indicate it's an advanced API), or replacing it with a friendlier API, although that's obviously a long shot this late in the game.

[WIStudent]

As a compromise, in my case I would already be satisfied with a method that returns all cached variables for a query, like for example

cache.getQueryVariables<T = any>(queryName: string): T[]

That way, the update function could look like this

update (cache) {
  cache.getQueryVariables<EntriesVariables>('entries').forEach(variables => {
    const read = cache.readQuery<Entries, EntriesVariables>({
      query,
      variables
    });
    const data = modify(read);
    cache.writeQuery<Entries, EntriesVariables>({
      query,
      variables,
      data
    )};
  });
}

[jedwards1211]

@WIStudent that would work, I also just proposed an updateQuery method that would do readQuery, modify callback, then writeQuery, but potentially for all cached variables (link above).

[jedwards1211]

@ivan-kleshnin

We never use readQuery and writeQuery at the team because they couple every freaking part of the system together.

Couldn't have said it better. At first I was going to say there should be a central place for us to declare what to update after a given mutation, but then I realized, it's already possible in userland to put an updateAfterCreateFoo function in its own module and import it and use it anywhere you createFoo. Then I also realized, doing so would be better for code splitting than declaring all updaters in the chunk that creates the store...

In any case you might be interested in my [apollo-magic-refetch package[(https://github.com/jcoreio/apollo-magic-refetch) for doing things in a decoupled way.

[benjamn]

Greetings all! I'm just coming back from a nice four-day weekend where I didn't look at my work computer at all, hence the delay.

Thanks to your feedback (especially @dmitrybirin's #6289 (comment) above, and @darkbasic's point about not being able to test other new AC3 features until cache.modify is restored) I'm persuaded that cache.modify is an important piece of the API for advanced users, and I think it can be salvaged—but not without some tweaks.

In addition to reverting this PR, here's a list of changes I think we need to make to smooth out the cache.modify story:

• Make cache.modify take named options rather than positional parameters. The positional parameter story was already showing its clumsiness in Make dataId parameter of cache.modify optional. #6178, and named options pave the way for cache.modify to take options like options.broadcast (implemented for cache.write{Query,Fragment} in Allow silencing broadcast for cache update methods. #6288).
• Upgrade the readField helper to align with the new ReadFieldOptions API for options.readField in read and merge functions, as implemented in Allow calling readField with ReadFieldOptions. #6306. I skipped cache.modify in that PR because I thought we were going to be removing it, but of course readField should work the same everywhere.
• Deemphasize toReference, and instead provide a pattern capable of handling the edge cases I described in Remove experimental cache.modify method. #6289 (comment). My current plan is to allow cache.writeFragment to be called without options.id, allowing the cache to infer the ID from options.data. Since cache.writeFragment currently returns nothing, we have some freedom to make it return something more useful, such as a Reference to the object that was just written into the cache! With these improvements, I believe cache.writeFragment can handle the use cases where toReference was struggling, while also supporting field arguments, variables, merge functions, etc. Best of all, the writeFragment API already exists, so the incremental documentation/explanation burden should be relatively small.
• Improve transactional broadcast behavior for reentrant cache update methods. If we're going to be encouraging calling methods like cache.writeFragment from within cache.modify modifier functions, then we need to make sure all cache update methods can be safely called while other update operations are in progress. Specifically, calling cache.writeFragment from within a modifier function should not trigger a broadcast in the middle of the larger cache.modify operation, because the larger operation will handle broadcasting when it finishes.
• Provide options.cache (the InMemoryCache instance) to read, merge, and cache.modify modifier functions. A small ergonomic improvement, but having access to options.cache makes it easier to call options.cache.writeFragment and other methods when you need them.

Putting everything together, here's an improved/expanded version of the example I used above:

const newTodo = {
  __typename: "Todo",
  id: ...,
  description: "Reinstate cache.modify with an improved API",
  completed: false,
};

cache.modify({
  modifiers: {
    todos(existing: Reference[], { cache, readField }) {
      // Per my proposal above, it should be possible to call cache.writeFragment
      // without knowing the ID, as long as cache.identify(newTodo) works.
      const newTodoRef = cache.writeFragment({
        data: newTodo,
        // This fragment only needs to describe the contents of newTodo,
        // not any of the other objects in this TodoList.todos field.
        fragment: gql`fragment NewTodo on Todo {
          id
          description
          completed
        }`,
      });
      // The data have been written, so we can skip appending newTodoRef
      // to the existing array if it already exists.
      if (existing.some(ref => readField("id", ref) === newTodo.id)) {
        return existing;
      }
      return [...existing, newTodoRef];
    },
  },

  // Optional when targeting the ROOT_QUERY object.
  id: cache.identify({
    __typename: "TodoList",
    id: ...,
  }),

  // True by default, but worth reiterating: this call to cache.modify
  // should trigger only one broadcast, despite using writeFragment
  // within the `todos` modifier function.
  broadcast: true,
});

I'm sure some additional nuances will become apparent as we get this implemented, but that's the general plan so far.

We are aiming to have a release candidate by the end of this week (May 29th), including this work and a handful of other new features. Once the RC period starts, we will not add (or remove!) any new features before the AC3 launch, but we will continue fixing bugs and improving the documentation.

[jrnail23]

@benjamn, this is great news. Thanks for being so receptive and responsive on this stuff!

[jedwards1211]

The more I think through how I would use cache.modify the more ungainly it seems. I think the following would be so much simpler to use:

const newTodo = {
  __typename: "Todo",
  id: ...,
  description: "Reinstate cache.modify with an improved API",
  completed: false,
}

cache.updateFragment({
  fragment: gql`
    fragment todos on TodoList {
      id
      todos {
        id
        description
        completed
      }
    }
  `,
  id: ...,
  updater: ({ data }) => ({
    ...data,
    todos: [...data.todos, newTodo],
  })
})

Though I guess it's possible to implement a method like this with userland code (I wonder if I will ever end up needing to use cache.modify?)

However I think the most crucial missing feature is still bulk updates for all variables a query has been made with as discussed in my updateQuery feature request.

[jedwards1211]

Actually I forget that cache.modify apparently can operate on all instances of a field regardless of args/variables. But being able to do that but not get passed the associated args/variables and the overall API is just wonky.

[benjamn]

@jedwards1211 No need to remind me that cache.modify is ungainly!

However, I am still convinced that cache.modify has a role to play, and I agree with you that updateFragment could be implemented in user-land (for now, at least), as an ergonomic improvement to the readFragment/transform/evict/writeFragment pattern.

I was tempted to bring back a version of cache.modify where you would pass in a query or fragment to achieve something similar to your updateFragment idea, with modifier functions operating on GraphQL result objects rather than internal cache data. One of the points that dissuaded me was @dmitrybirin's #6289 (comment), which shows how difficult it can be to come up with an appropriate fragment todos on TodoList {...} that applies to the whole list, unless you have special knowledge of what types might be stored in that list. Such "special knowledge" implies some degree of coupling, as @ivan-kleshnin pointed out in #6289 (comment).

By contrast, cache.modify lets you concern yourself with just the new data that you're inserting, or the specific criteria you're using to remove data, and doesn't require reading out all the other objects in the list that you don't care about modifying. These benefits are almost too subtle to justify cache.modify—and that's why we almost removed it!—but I believe they are real benefits.

[jedwards1211]

Those are good points...mainly I just hope to see the cache store the variables and args unserialized, and pass them to some read or modify methods, so that we can perform updates that are currently impossible. Do you think that's in the near future?

[benjamn]

@jedwards1211 I would like to see cache.modify functions receive options.args, somewhat like read and merge functions currently do.

For cache.modify specifically, I'm imagining options.args would be the most recent arguments object (possibly null) from the last time the field's value was updated, rather than anything more complicated, like a history of arguments or variables.

Implementing options.args would be somewhat tricky right now (which mostly means we are unlikely to get to it before AC3 is released), because field arguments are not currently saved (or reversibly encoded) in the cache's internal representation. However, I don't see any immediate problems (other than slightly increasing cache size) with the EntityStore saving the original, unfiltered arguments alongside their corresponding field values somehow, so the arguments could be recovered without any parsing of storeFieldName strings.

Historically, storing the original arguments alongside field values has not been necessary for reading and writing queries or fragments, because the given query/fragment plus variables allows computing specific, unambiguous arguments for any field. What you've sketched so far with cache.updateFragment in #6335 seems like it follows this unambiguous path, too, leaving cache.modify and cache.evict as the only two APIs that modify the cache without the guidance of a query/fragment plus variables, potentially acting on multiple field values per field name, which might have been written using different arguments.

Both modify and evict are new in AC3, so I'd like to give them some time to bake, so we can see how people (want to) use them. That said, I'm pretty sure the options.args idea is technically feasible, without a major @apollo/client version bump.

In the meantime, please feel free to share any compelling use cases that options.args (or something similar) could solve.

[jedwards1211]

What if multiple queries with different arguments for a field are active or cached though? Would the updater be called for each one?

[tomaswitek]

@benjamn I appreciate your work very much. But right now it's very misleading for those who upgrade. Before upgrading I've done my job and checked the docs and googled some articles, modify was the main motivation for me to upgrade after reading docs and blog posts. If you google Apollo Client v3 the first result is documentation. The first example for a local state management in docs is implemented using modify: https://www.apollographql.com/docs/react/v3.0-beta/data/local-state/#local-resolvers. The second result is this blog post: https://www.apollographql.com/blog/first-impressions-with-apollo-client-3-2ae2a069ab2f. One of main selling points here is again modify. I have to say I was very surprised to see this exception after upgrade: modify is not a function. Can we pls update those webpages and save time for feature upgraders?

[darkbasic]

@benjamn thanks, your proposed modifications sound good.

Just a small question regarding your example:

    todos(existing: Reference[], { cache, readField }) {
      const newTodoRef = cache.writeFragment({...});
      // The data have been written, so we can skip appending newTodoRef
      // to the existing array if it already exists.
      if (existing.some(ref => readField("id", ref) === newTodo.id)) {
        return existing;
      }

Why wouldn't you need to add newTodo to the existing array? It has been written to the Todo cache, of course, but being part of todos is another thing.

We are aiming to have a release candidate by the end of this week (May 29th), including this work and a handful of other new features

If you're really commited to release an RC before the end of May, so be it. Otherwise I think we could use some more time to stabilize before v3 gets exposed to a broader public, especially things like this: #6183 (comment)

Regarding args, I see some use cases which could benefit from a more fine grained control. Let's say our todos query accepts a searchFilter argument and we have cached searches for the following terms: modify, cache.modify and cache.modify(). When we add our new todo called "Reinstate cache.modify with an improved API" we should add it to the first two cached queries, but not the third one (which doesn't match).

[benjamn]

Why wouldn't you need to add newTodo to the existing array?

The newTodoRef does get added to the array by the return [...existing, newTodoRef] statement on the last line of the function, if it wasn't found in the existing array already. You could skip the existence check, but you might end up with duplicate references in the TodoList.todos array.

[danReynolds]

@benjamn thanks for looking into another stab at the modify API! I think it's really valuable. One question I have though is for a case like say firing a mutation to create a new todo, then wanting to append that todo to your existing cached todos query.

With the old modify API, that could be done like this:

const ADD_TODO = gql`
  mutation AddTodo($type: String!) {
    addTodo(type: $type) {
      id
      type
    }
  }
`;

function AddTodo() {
  const [addTodo] = useMutation(
    ADD_TODO,
    {
      update(cache, { data: { addTodo } }) {
        cache.modify('ROOT_QUERY', {
          todos(existingTodos, { toReference }) {
            return [...existingTodos, toReference(addTodo)]
          }
        });
      }
    }
  );
}

Which worked great! We appended our added todo to the list. But with the writeFragment approach, I need to know which fields exist for the created entity and specify those in the fragment. My ADD_TODO mutation lists them, but now I'd need to write a fragment for them, and make that fragment available to wherever I do cache.modify.

This also makes it impossible if I want to do something generic like a rule that any time a Todo is created, add it to the todos list. There may be multiple mutations for addTodo that take different field subsets. The generic update handler wouldn't know what fragment to generate for the entity it is given.

[benjamn]

@danReynolds That usage of toReference works because the result of the mutation (including data.addTodo) is automatically written into the cache (using the ADD_TODO mutation document), so the Reference returned by toReference(addTodo) points to existing data in the cache. I am not planning to remove the toReference helper, since it's still useful for situations like this (creating references without updating cache data).

[danReynolds]

@danReynolds That usage of toReference works because the result of the mutation (including data.addTodo) is automatically written into the cache (using the ADD_TODO mutation document), so the Reference returned by toReference(addTodo) points to existing data in the cache. I am not planning to remove the toReference helper, since it's still useful for situations like this (creating references without updating cache data).

Ah I mistook deemphasize for discourage, sounds good!

[benjamn]

Alright everybody, please head over to #6350 if you want to continue this discussion!

[benjamn]

New cache.modify implementation out now in -beta.51: #6350 (comment)