Incorrect resolution of nested `@client` fields in v4

[broyde]

Issue Description

I have a query with client field:

query {
  child {
    clientField @client
    normalField
  }
}

And typePolicies:

typePolicies: {
  Child: {
    fields: {
      clientField: (value = 1) => value,
    },
  },
},

When I do const {data} = await client.query({query}), I expect data.child.clientField to be 1, but it's null instead.

I assume it may be helpful:

• With separated read and merge functions we can see that merge is called first in this case, and it writes null. That's the problem.
• With root fields instead of nested ones, it works properly.
• With @client only fields it works properly too.

Link to Reproduction

https://github.com/broyde/apollo-nested-client-field

Reproduction Steps

I've created simple jest tests to show the problem.

1. git clone https://github.com/broyde/apollo-nested-client-field.git
2. cd apollo-nested-client-field
3. npm i
4. npm test
5. See the test nested-client-field.test.js and how it fails:

FAIL ./nested-client-field.test.js
  ● Console

    console.log
      merge { cachedValue: undefined, value: null }

    console.log
      read null

  ● Should resolve nested @client fields properly

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

      Object {
        "__typename": "Child",
    -   "clientField": 1,
    +   "clientField": null,
        "normalField": 2,
      }

      49 |
      50 |   const {data} = await client.query({query});
    > 51 |   expect(data.child).toEqual({
         |                      ^
      52 |     __typename: 'Child',
      53 |     clientField: 1,
      54 |     normalField: 2,

      at Object.toEqual (nested-client-field.test.js:51:22)

@apollo/client version

4.0.5

[triagster]

Related issues:

• [4.0] LocalState warns and sets value to null on fields that define read function #12730

Was this useful? Use the reactions below to give feedback.

[jerelmiller]

Hey @broyde 👋

I think the issue here is that the value received as existing in read functions is actually null, not undefined. LocalState tries to ensure there is a value for every field upon resolution, so instead of leaving values as undefined, it sets them to null. That triagster link points to a PR that includes this in the changelog:

Note that read functions without a defined resolver will receive the existing argument as null instead of undefined even when data hasn't been written to the cache. This is because LocalState sets a default value of null when a resolver is not defined to ensure that the field contains a value in case a read function is not defined rather than omitting the field entirely.

In your case, I think the default isn't picked up for this reason. Can you try updating your read function to use nullish coalescing operator instead?

typePolicies: {
  Child: {
    fields: {
      clientField: (value) => value ?? 1,
    },
  },
},

Edit: I can confirm that using value ?? 1 here resolves the issue using your reproduction.

[jerelmiller]

Also if you want some background on why this value is null, see this comment in that PR which hopefully provides some context on why this is the case.

That said, this probably should be in the documentation so I'll see if I can get something in there today.

[jerelmiller]

I want to document one more thing here so that this information is available in case others stumble upon this issue.

You might be wondering why root fields default to undefined instead of null, unlike child fields. The answer comes down to how cache reads work.

When you're using a fetchPolicy that reads from the cache, Apollo Client will try and fulfill that query from the cache before trying to execute it on the network. You can think of LocalState as part of the network in the context of how queries are executed when there are @client fields present as it runs after the initial cache read occurs. If the cache read can fulfill the data requirements for the query, it returns the query immediately.

If you're using @client on root fields with a read function, that read function will run as a result executing the initial cache read. The existing argument is set to undefined here because the field has no value actually written to the cache. If this initial cache read can't fulfill the data requirements for that query (i.e. because you have no read function or your read function returns undefined), it will try and execute the result from the network. This is where LocalState comes in, again because you can think of it as part of the "network" layer.

LocalState tries to fulfill @client fields by running local resolvers. If a local resolver isn't provided for a particular @client field, LocalState ensures there is a value returned for that field. It will try and fulfill that value from the cache if it can, but if a value isn't written to the cache, it sets the value to null. That null ensures future cache reads don't result in a cache miss (which would be a problem if that query also included other server fields as your query would execute on the network much more often than you'd want it to).

Now let's look at this through the lens of a child @client field on a server field. The cache read will run and try and fulfill the data from the cache. If the server field isn't written to the cache, it will try and execute the query from the network. Child read functions don't run in this case since there is no data written to the cache for the parent field. Apollo Client will execute the query on the network, which includes sending server fields to the server, then taking the result of the server-executed query and sending that value through LocalState to fill in @client fields. Much like root @client fields, it will provide null for fields that don't have a local resolver or written cache value. The combination of the server value and the LocalState value is what gets written using to the cache, hence why your merge function runs with a null value here.

Hopefully you can see why there is the difference in null vs undefined in each of these cases. Again, the fix here is to avoid using argument default values and use the nullish coalescing operator in the read function itself.

Hope this helps!

[broyde]

Hi @jerelmiller. Thanks for fast response.

LocalState tries to ensure there is a value for every field upon resolution, so instead of leaving values as undefined, it sets them to null.

I didn't know. I think it would be better to have some errors or, at least, warnings if some resolvers are required but not defined.

In your case, I think the default isn't picked up for this reason. Can you try updating your read function to use nullish coalescing operator instead?

I don't think this is the reason. Logs show that the typePolicies merge function is called first, and it writes null into the cache. Then, read just reads it.
I need to distinguish between undefined (the value is missing in the cache) and null (the cache has this value, and it's null).
It would be nice to have consistent behavior for root and nested fields.

[jerelmiller]

@broyde I've played around with this a bit more and think I have a good compromise here. I've put up #12934 to try and help here.

This PR makes a couple changes:

• It no longer tries to set the default value to null if a read function is defined for a field (allowing default parameters to work).
• It warns if neither a read function or local resolver is defined for the field and sets the value to null (preserving the functionality that exists to ensure you don't get future cache misses)
• Warns and sets the field value to null if a no-cache fetch policy is used when using a read function.

I've appreciate you trying out the PR build and let me know if you have any feedback here.

npm i https://pkg.pr.new/apollographql/apollo-client/@apollo/client@12934

[broyde]

@jerelmiller What I observe with this PR:

• The test now passes
• However, I don't see any warnings when removing the typePolicies
• I also don't see any warnings with no-cache. Additionally, it doesn't call the read function at all

I still can't understand the reasoning behind making the behavior inconsistent based on fetchPolicy. I believe the read function should be called even with no-cache, but the cache it reads from should be empty in this case.

With this solution, it seems I'll need to migrate from typePolicies back to local resolvers, which I worked hard to remove when they were deprecated.

[jerelmiller]

• However, I don't see any warnings when removing the typePolicies
• I also don't see any warnings with no-cache.

That's odd. Can you double check you're not running a production build? Those runtime warnings are stripped out of the build when __DEV__ is false so maybe that's what you're seeing? It's quite clearly tested:

apollo-client/src/local-state/__tests__/LocalState/base.test.ts

Lines 588 to 619 in 700836e

	test("warns and sets value to null when a resolver is missing for an `@client` field and a read function is not defined when using InMemoryCache", async () => {
	using _ = spyOnConsole("warn");
	const document = gql`
	query {
	foo @client
	}
	`;
	const client = new ApolloClient({
	cache: new InMemoryCache(),
	link: ApolloLink.empty(),
	});
	const localState = new LocalState();
	await expect(
	localState.execute({
	document,
	client,
	context: {},
	variables: {},
	remoteResult: undefined,
	fetchPolicy: "cache-first",
	})
	).resolves.toStrictEqualTyped({ data: { foo: null } });
	expect(console.warn).toHaveBeenCalledTimes(1);
	expect(console.warn).toHaveBeenCalledWith(
	WARNINGS.MISSING_RESOLVER,
	"Query.foo"
	);
	});

This is the only thing I could think of that could prevent that warning from being shown (that is unless you have a value written to the cache already for that field, in which case LocalState won't be run)

I believe the read function should be called even with no-cache

The reason it shouldn't is because its at odds with the stated behavior of that fetch policy. From the docs:

Similar to network-only, except the query's result is not stored in the cache and external cache updates are completely ignored.

The relevant part for network-only here:

Apollo Client executes the full query against your GraphQL server, without first checking the cache

no-cache by its nature doesn't read from/write to the cache, so performing cache reads/writes don't make sense here. If local fields were to suddenly interact with the cache for a no-cache query, then I'd argue it's inconsistent with how server queries work (i.e. read functions don't run for no-cache server queries since the result isn't written to/read from the cache.)

Aside from that though, there would be no way to actually handle the query you've provided in the description with a no-cache query because a nested @client field requires that the server field is written to the cache in order to run the read function. A no-cache query will always result in a missing value for that field in this case. LocalState is just making sure it has a non-undefined value here so that the result is a complete result.

With this solution, it seems I'll need to migrate from typePolicies back to local resolvers, which I worked hard to remove when they were deprecated.

Could you expand on why you feel this way? The solution in the PR makes it easier to use read functions (by allowing default parameters to work correctly) and provides more helpful warnings so I'm not quite sure how it encourages you to move to local resolvers.

when they were deprecated

Apologies for this by the way. For a while we planned to remove local resolvers entirely, hence the short deprecation, but we heard from quite a few in the community that local resolvers are actually quite useful, so we reversed that decision. Initially Apollo Client 4 was going to move local resolvers to the link chain, but we quickly discovered that working with read functions was much trickier doing it this way and we want to continue supporting both patterns, hence why localState is an option on the ApolloClient constructor.

All that to say, you're free to use either read functions or local resolvers! We plan to support both patterns for the forseeable future. We've actively invested in local resolvers in v4 to make them better and easier to use (better error handling, a new codegen plugin for better TypeScript types, etc.) so we encourage people to use them when it makes sense. Don't feel like you need to avoid them!

[jerelmiller]

Oh and one more point for clarification... LocalState doesn't actually run read functions directly. It can't because it needs to work with 3rd party caches and 3rd party caches aren't guaranteed to have anything like type policies. Instead LocalState relies on the cache read/writes when queries are executed in order for those read functions to work. Hope this helps explain why no-cache plays a role here.

[broyde]

That's odd. Can you double check you're not running a production build? Those runtime warnings are stripped out of the build when __DEV__ is false so maybe that's what you're seeing? It's quite clearly tested:

I didn't build it; I just tried it in a test environment.
But now I'm trying it with a real project, and I see a lot of them in the dev build.
By the way, I've found a case where it seems ok to use a @client field without defining a typePolicy/resolver, but I get a warning. The parent field policy writes the clientField to the cache:

typePolicies: {
  Query: {
    fields: {
      item: {
        merge: (_, value, {toReference}) => toReference({...value, clientField: 1}, true),
      }
    }
  },
  // Defining this `read` function helps avoid the warning, but it seems weird and unnecessary:
  Item: {
    fields: {
      clientField: (value) => value,
    }
  }
}

The reason it shouldn't is because its at odds with the stated behavior of that fetch policy. From the docs:

Similar to network-only, except the query's result is not stored in the cache and external cache updates are completely ignored.

I thought it worked in v3, but now I've checked with 'no-cache', and it doesn't return any values for @client fields. I've inspected one of these cases and it's fine there. The reason why the @client field is used is simply because we have a fragment that's used in multiple queries.

With this solution, it seems I'll need to migrate from typePolicies back to local resolvers, which I worked hard to remove when they were deprecated.

Could you expand on why you feel this way? The solution in the PR makes it easier to use read functions (by allowing default parameters to work correctly) and provides more helpful warnings so I'm not quite sure how it encourages you to move to local resolvers.

Oh, I meant v4 as a whole. Even with this PR, it still won't work the same way as it did with v3.
But the PR itself is definitely an improvement and partially restores the behavior from v3.

Thank you. I appreciate your support.