Urql: (graphcache) - Resolvers are run for readQuery

[olistic]

[email protected]
@urql/[email protected]

Steps to reproduce

1. Have a Graphcache configured with resolvers and updates:

cacheExchange({
  resolvers: {
    Foo: {
      date: ({ date }) => parseISO(date), // Convert string `date` to Date object.
    },
  },
  updates: {
    createFoo: (result, args, cache) => {
      const {
        createFoo: { foo },
      } = result;
      cache.updateQuery(
        {
          query: FooListQuery,
        },
        (data) =>
          data && {
            ...data,
            foos: [foo, ...data.foos],
          },
      );
    },
  },
});

1. Execute createFoo mutation with at least one other foo already in Graphcache's cache.

Expected behavior

No errors are thrown, both the existing foo and the new foo end up in the cache.

Actual behavior

An error is thrown because parseISO in the resolver config tries to parse a date that is already a Date object. This happens because the data argument provided to updateQuery has already been modified by the custom resolvers, and it's then written to the cache in the modified form.

As a workaround, I'm converting the date back to a string before writing to the cache in updateQuery, but this doesn't seem ideal.

If this is not the intended behavior, I'd like to submit a PR. Would it make sense to skip the custom resolvers when reading the cache for updateQuery?

[kitten]

This is an interesting edge case! I’m tempted to say that it’s unlikely that it’s undesirable behaviour to have the resolvers run for queries. I personally expect this, because resolvers can do much more, and this can ultimately stack in unexpected ways.

A resolver can also link up entities after all, and we wouldn’t want this behaviour to be disabled in the cache. If a resolver is simply used to directly get to an entity, because e.g. it’s likely to have been queried by a list but not by a details query, then the updater should still work.

Then again, there are cases where this may cause errors;

I’m thinking a common issue may be pagination where a resolver then delivers one merged page, while the updater is attempting to update a single page. In that regard it’s unexpected.

So a solution that only applies to resolvers being skipped for “scalar” fields is unlikely to be sufficient. But a solution that skips all resolvers may also cause unexpected behaviour.

I suppose we could argue that if a link doesn’t exist, a resolver imitates it, and the entity does exist, then it’s also likely that an update would already not be manual due to normalised data.

So I think the arguments are in your favour, but I’d love to leave this open for now to give us time to think whether it’d break anything.

cc @JoviDeCroock @wgolledge

[JoviDeCroock]

I honestly think that disabling resolvers for writes isn't a bad thing since we'll read right after the write either way. I'd like to see if we disable it if it affects our tests, we shouldn't write a new result to the cache ever, a resolver should only return an altered result to the result in my opinion.