Reinstate cache.modify with improved API.

[benjamn]

Thanks to developer feedback (see #6289 (comment)) we have decided that cache.modify is an important piece of the API after all (at least for certain advanced use cases), and we believe it can be salvaged—but not without some tweaks. In addition to reverting PR #6289, this PR makes the following changes to improve 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). With this PR,cache.writeFragment can be called without options.id, allowing the cache to infer the ID from options.data. Calling cache.writeFragment also returns a Reference to the object that was just written into the cache. With these improvements, cache.writeFragment should be able to handle most of 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:

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

cache.modify({
  // 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,

  fields: {
    todos(existing: Reference[], { cache, readField }) {
      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];
    },
  },
});

@hwillson (and @stemmlerjs if you have time for a review): I definitely recommend a commit-by-commit approach to reviewing this PR.

[benjamn]

That bundlesize CI failure is not a surprise, since we're reinstating a nontrivial feature.

[benjamn]

One of the many benefits of using named options is that we can distinguish at runtime between options that were not provided at all and options that merely happen to have undefined values.

[darkbasic]

Awesome, I will definitely try this one so I'll we able to give some feedback on the other pending issues as well!

[hwillson]

This looks awesome @benjamn - thanks for thinking all of this through so thoroughly and carefully! 🎉 The code changes all look great (I'll do some extra application testing very shortly and let you know how that all turns out). LGTM!

[tafelito]

@darkbasic because I was having many issues when I tried to upgrade so I opt out for now but I will

btw, in your example you are doing

return {
    ...connection,
    edges: connection.edges.filter(
        ({node}) => node.__ref !== deletedRef.__ref,
    ),
};

why don't you do this instead?

return {
    ...connection,
    edges: connection.edges.filter(
        ({node}) => readField("id", node) !== mutationData.removeMatch,
    ),
};

and you avoid using the toReference

[darkbasic]

@tafelito the main reason is because I wanted to showcase how ugly the __ref prop looks like and ask if it's really necessary or if it could be moved to the root object instead. The second reason, which could be a complete exaggeration on my side, is that I'm not sure about how expensive readField could be. So instead of calling it each time in a very long array I chosen to simply call toReference once. If I recall correctly Apollo doesn't follow the Identity Map pattern for the references, that's why I had to match the __ref prop instead of the whole object.

[darkbasic]

By the way we've talked alot about how to improve the Reference experience for the upcoming v4 of Mikro-ORM, so if anybody is interested in the topic I suggest to have a look at mikro-orm/mikro-orm#145 and mikro-orm/mikro-orm#214

[benjamn]

@darkbasic The { __ref } objects are shaped that way because they need to be trivially JSON-serializable to keep cache.extract and cache.restore fast (no pre/post-processing), and they also must not collide with any actual GraphQL properties. Since user-defined GraphQL field names are not allowed to start with a double underscore, __ref satisfies these constraints. I do not share your concern about aesthetics here, and I'm not sure what you mean by moving the __ref property "to the root object." The value of the __ref property is just a string (the ID of the entity), and we definitely cannot represent references using just strings, since there are no convenient limitations on the possible contents of string values in GraphQL.

Also, just so you know, isReference does not check that the reference actually points to anything, only that it has the right { __ref } shape. It's mostly useful in TypeScript because the isReference return signature (value is Reference) allows TS to do type narrowing in control flow paths where isReference(value) returned true.

[benjamn]

Here's some background on why cache.evict evicts data from all layers (optimistic and non-optimistic), rather than just the topmost layer, which I suspect may be the source of your (cache.evict-specific) optimistic rollback problems: #5773

[darkbasic]

Here's some background on why cache.evict evicts data from all layers (optimistic and non-optimistic), rather than just the topmost layer

As I said I can be fine with the fact that cache.evict does this (the name somehow implies it and anyway if you want the other behaviour you can always use cache.modify along with DELETE), but we should have at least some context about which kind of update we are performing. I may want to fire a cache.evict only if I received the response from the server, but actually inside the update method of useMutation there is no way to know if you are performing an optimistic update or not. What I'm proposing is adding a boolean field called isOptimistic so you can do something like this:

const [removeMatch, {error: removeMatchError}] = useMutation<
  RemoveMatchMutation,
  RemoveMatchMutationVariables
>(RemoveMatchDocument, {
  update(cache, {data}, {isOptimistic}) {
    [...]
    if (!isOptimistic) {
      cache.evict([...]);
    }
  },
});

[darkbasic]

Also, just so you know, isReference does not check that the reference actually points to anything, only that it has the right { __ref } shape

I kind of imagined this, but unfortunately it's still necessary to narrow the return type from toReference: "" | Reference | undefined. The error message is wrong but I couldn't figure out how to better describe it, because I'm not sure when toReference can actually return "" | undefined.

[benjamn]

Ok, isReference is a fine way to check that toReference returned a Reference rather than undefined. And, just to be clear, toReference might return undefined if it fails to identify the object.

The empty string thing is a bug that I will fix momentarily (in the next beta) by rewriting this code:

  public toReference = (
    object: StoreObject,
    mergeIntoStore?: boolean,
  ) => {
    const [id] = this.policies.identify(object);
    const ref = id && makeReference(id);
    if (ref && mergeIntoStore) {
      this.merge(id!, object);
    }
    return ref;
  }

to this:

  public toReference = (
    object: StoreObject,
    mergeIntoStore?: boolean,
  ) => {
    const [id] = this.policies.identify(object);
    if (id) {
      const ref = makeReference(id);
      if (mergeIntoStore) {
        this.merge(id, object);
      }
      return ref;
    }
  }

[benjamn]

What if the mutation inserts multiple rows, do you need to loop over every new row and call writeFragment (or toReference) or is there any way to write multiple fragments at once?

@tafelito That's an interesting question!

Short answer: you definitely can call toReference and cache.writeFragment in a loop, and performance should be fine in most cases (measure before optimizing!), especially since cache.writeFragment now behaves as if you passed broadcast: false when called inside a cache.modify function (thanks to 09280f4).

The toReference function should be quite cheap (in part because it skips so many of the steps that cache.writeFragment gets right, like normalizing objects and handling field arguments). In other words, if toReference is adequate for your needs (e.g. you know that all the fields are simple scalars), then I don't see any problem with calling toReference in a loop.

⚠️ Note: everything below here is speculative/exploratory and not an endorsement/recommendation! 🚨

As for cache.writeFragment, I suppose you could do something like

// XXX Not recommended; just an idea!
const tempRef = cache.writeFragment({
  fragment: gql`fragment Temp on Temporary {
    people {
      firstName
      lastName
    }
  }`,
  data: {
    __typename: "Temporary",
    id: "???", // Not sure about the best fake ID to use here.
    people: [
      // Be sure that these objects all have __typename and id / key fields!
      firstPerson,
      secondPerson,
      thirdPerson,
      ...
    ],
  },
});

if (tempRef) {
  const people = readField<Reference[]>("people", tempRef);
  cache.evict(tempRef.__ref);
  // Do something with people...
}

I'm not ready to recommend this pattern in any way, but I imagine it would push the looping work down into the writeFragment processing, which might reduce any overhead per iteration.

Alternatively, using cache.writeQuery should work too:

// XXX Not recommended; just an idea!
const rootRef = cache.writeQuery({
  // Be sure to use a root query field that isn't used elsewhere!
  query: gql`query {
    temporaryPeople {
      firstName
      lastName
    }
  }`,
  data: {
    temporaryPeople: [
      // Be sure that these objects all have __typename and id / key fields!
      firstPerson,
      secondPerson,
      thirdPerson,
      ...
    ],
  },
});

const people = readField<Reference[]>("temporaryPeople", rootRef);
cache.evict({ fieldName: "temporaryPeople" });
// Do something with people...

This works because cache.writeQuery is essentially the same as cache.writeFragment, except that it always writes to the ROOT_QUERY object.

[darkbasic]

As for cache.writeFragment, I suppose you could do something like

I guess this would work, but it really looks ugly :)

@benjamn the more I think about it the more I think that we should expose the query args inside cache.modify. Until now I simply gave up on updating the GetMatches queries whenever a user creates a new match because there could be filters like startsAfter: Date or minPartecipants: Int, but if we expose the args it would be really easy to accomplish this. I'm starting to think that this could be a game changer big enough to justify a slight delay in the release date.

[darkbasic]

@benjamn I even started to think about stupid ways to work it around if that won't happen in time for Apollo 3.0, like returning the list of args in the server response along with the Connection object I use for pagination. Don't let me do this, please :)

[benjamn]

@darkbasic I think I agree about passing options.args to cache.modify! I'll give some thought to the right implementation today.

[tafelito]

@benjamn I actually think that's a good idea. I like that the writeFragment would do the loop and I just have to pass the data to it

[benjamn]

Sure, and it's a lot less ugly if you hide it in a reusable helper function!

[tafelito]

One thing I noticed is that even if I modify the cache with either writeFragment or toReference on the updatefn, the original query still re run and fetches the server, I don't think that's supposed to happen, right?

[darkbasic]

@benjamn I think that it's worth mentioning in the docs that if you're using typePolicies (like this #6278) then cache.modify won't trigger the methods attached to those fields: instead you will have to update the fields which feed them in typePolicies.

[darkbasic]

@benjamn I think we also need a pointer to the current object (the same which readField defaults to) inside the field's Modifier object. Since I need to do the same update in multiple places (wouldn't be needed with cache redirects, but I can't assume that would always be the case) I would like to be able to reuse the same function: unfortunately there is no way to know which object is actually calling it unless you're manually passing such parameter.

((cache as unknown) as InMemoryCache).modify({
  id: cache.identify({
    __typename: 'GroupChat',
    id: chatOrGroupId,
  }),
  fields: {
    messages: handleMessages,
  },
});

((cache as unknown) as InMemoryCache).modify({
  fields: {
    messages: handleMessages,
  },
});

In the previous example I will have to match the id from the cache.modify args for the ROOT_QUERY field, but not for the GroupChat one.
I think exposing the current object would come in handy.

[benjamn]

For anyone still in need of something like options.args, here's a workaround that recently occurred to me: #6394 (comment)

[benjamn]

@darkbasic Re: #6350 (comment), we are planning to add options.id for read, merge, and modify shortly after the AC3 launch (v3.1, most likely). Also, thanks to #6478, you will be able to do options.toReference(options.id) to make a Reference to the current object.

[behivetech]

I know there's a lot to manage with this library, but could you pretty please with sugar on top update the docs. I'm trying to do the update with useMutation on @apollo/react-hooks v4.0.0 and I get the message... TypeError: cache.modify is not a function. It's a bit frustrating trying to go by the docs that instruct things that don't work and have to spend time searching around for why they don't. BUT I do really appreciate all the hard work. This library is pretty amazing and thank you for that.

Here's the reference I'm talking about in the docs...
https://www.apollographql.com/docs/react/caching/cache-interaction/#example-updating-the-cache-after-a-mutation