Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with @client normalization, optimisticResponse, and field policies #7941

Open
wallzero opened this issue Apr 2, 2021 · 6 comments
Open

Comments

@wallzero
Copy link

wallzero commented Apr 2, 2021

Intended outcome:

I am trying to tack on a local field to an otherwise normal query and expect the local field to be normalized and the cache to create a ref. I have a read field resolver providing a default value.

Actual outcome:

The default value provided by the read field resolver is not normalized. Inspecting the parent, it contains no reference to any normalized object returned by the read field policy. This means that while the query for the @client field was returned, it is unable to update the field via cache.modify to persist any changes.

How to reproduce the issue:

Basically I am querying a graph-like object. I also will need a local @client

Original schema:

type Graph {
	graphId: ID!;
	name: String;
}

Extended schema to accommodate local settings:

type GraphSettings {
	graphId: ID!
	zoom: Int!
	x: Int!
	y: Int!
}

extend type Graph {
	settings: GraphSettings!
}

My field policy:

... Inside Graph ...
settings: {
		keyArgs: ['graphId'],
		merge: true,
		read (
			existingData,
			{
				args,
				canRead,
				toReference,
				variables
			}
		) {
			if (existingData) {
				return existingData;
			}

			const graphId = args?.graphId || variables?.graphId || null;

			const ref = toReference({
				__typename: 'GraphSettings',
				graphId
			});

			if (canRead(ref)) {
				return ref;
			}

			const defaults = {
				__typename: 'GraphSettings',
				graphId,
				zoom: 1,
				x: 0,
				y: 0
			};

			return defaults;
		}
	}

I then query the graph:

query GraphPreload (
	$graphId: ID!
) {
	graph (
		graphId: $graphId
	) {
		graphId
		name
		settings @client {
			graphId
			zoom
			x
			y
		}
	}
}

And I can see my field policy is working and on first read it will return the defaults.

At some point, I want to mutate the GraphSettings:

mutation GraphSettingsUpdate(
		$graphId: ID!
		$graphSettings: GraphSettingsInput!
	) {
		graphSettingsUpdate(
			graphId: $graphId
			graphSettings: $graphSettings
		) @client {
			graphId
			zoom
			x
			y
		}
	}

Then I return a simple optimistic response from my mutate():

await mutate({
	optimisticResponse: {
		graphSettingsUpdate: {
			__typename: 'GraphSettings',
			graphId,
			zoom,
			x,
			y
		}
	},
	variables: {
		graphId,
		graphSettings: {
			zoom,
			x,
			y
		}
	}
});

This is where things become interesting. First, I tried creating a merge function in my field policy for the graphSettingsUpdate mutation:

/* Mutation graphSettingsUpdate field policy */
export default {
	keyArgs: ['graphId'],
	merge (
		existingSettings,
		newSettings,
		{
			cache,
			readField
		}
	) {
		/* ... try stuff ... */
	}
} as FieldPolicy<GraphSettings, GraphSettings, GraphSettings>;

This merge function is called but existingSettings is empty. newSettings returns a ref instead of a settings object, and readField can read updated values from that ref.

So I tried to readField the fields from the ref and cache.modify to update the settings but as I said above Graph does not have a settings reference in the cache and cache.modify will not create a new field. What is also strange is I can't find a GraphSettings object normalized in the cache either (not sure how readField is working then...)

So instead I tried cache.writeFragment to "force" a normalized reference. Basically the following:

	fragment GraphSettingsUpdate on Graph {
		graphId
		settings {
			graphId
			zoom
			x
			y
		}
	}
cache.writeFragment<GraphSettingsUpdateFragment>({
	data: {
		__typename: 'Graph',
		graphId,
		settings: {
				__typename: 'GraphSettings',
				graphId,
				zoom: 1,
				x: 0,
				y: 0
			}
	},
	fragment: GraphSettingsFragment,
	fragmentName: 'GraphSettingsUpdate',
	id: cache.identify({
		__typename: 'Graph',
		graphId
	})
});

This didn't work. I did not receive any errors, but nothing changes in my cache. I triple checked my cache.writeFragment and confirmed it even worked in other parts of my code; but within this merge function it does nothing. It also didn't make a difference what I return from this merge function.

Could I instead try cache.writeFragment in the settings merge field policy of Graph? I replaced merge: true with a copy of the graphSettingsUpdate merge function. It still did not create a normalized reference and was now also creating an infinite loop - repeatedly triggering the merge function.

So I thought why don't I just try forcing normalization on the first read in the settings read field policy of Graph? This didn't work. Trying to use cache.writeFragment inside a read field policy returns the following error:

Error
already recomputing

However, my Graph object in the cache now has a settings field! (Interesting the same cache.writeFragment seems to do nothing inside a merge function but succeeds (then throws an error) in a read function.) What happens if I try wrapping this cache.writeFragment in a try {} catch {}? Everything continues as normal and the settings are normalized. This is now what the read settings field policy looks like:

... Inside Graph ...
settings: {
		keyArgs: ['graphId'],
		read (
			existingData,
			{
				cache,
				args,
				canRead,
				toReference,
				variables
			}
		) {
			if (existingData) {
				return existingData;
			}

			const graphId = args?.graphId || variables?.graphId || null;

			let ref = toReference({
				__typename: 'GraphSettings',
				graphId
			});

			if (canRead(ref)) {
				return ref;
			}

			const settings = {
				__typename: 'GraphSettings' as 'GraphSettings' | undefined,
				graphId,
				zoom: 1,
				x: 0,
				y: 0
			};

			try {
				ref = cache.writeFragment<GraphSettingsCreateFragment>({
					data: settings,
					fragment: GraphSettingsFragment,
					fragmentName: 'GraphSettingsCreate',
					id: cache.identify({
						__typename: 'GraphSettings',
						graphId
					})
				});
			} catch (error) {}

			if (canRead(ref)) {
				return ref;
			}

			// eslint-disable-next-line no-console
			console.warn('Failed to find ref');

			return null;
		}
	}

However, even though I am returning the newly created ref, the Graph does not have a reference to GraphSettings in the cache.

Now that settings are normalized I should have been able to update them using cache.modify or cache.writeFragment from my merge functions, right? Nope. I confirmed both the Mutation graphSettingsUpdate merge field policy and the Graph settings merge field policy are called and cache.modify and cache.writeFragment fail to update the normalized GraphSettings.

So next I tried looking into my mutate function. One of mutate's options is to pass an update function, which is passed an instance of the cache:

/* ... mutate function options */
update (
		cache
	) {
		cache.modify({
			fields: {
				settings () {
					const settings = {
						__typename: 'GraphSettings',
						graphId,
						zoom,
						x,
						y
					};

					return settings;
				}
			},
			id: cache.identify({
				__typename: 'Graph',
				graphId
			})
		});
	}

And still nothing updates! Then, I thought maybe I should try just updating individual fields:

update (
		cache
	) {
		cache.modify({
			fields: {
				x () {
					return x;
				},
				y () {
					return y;
				}
			},
			id: cache.identify({
				__typename: 'GraphSettings',
				graphId
			})
		});	
}

And it works! When checking my cache, the graph settings were pushed to the normalized GraphSettings. I don't understand why modifying Graph failed, though. I then tried the above working cache.modify in my Mutation graphSettingsUpdate settings merge field policy just to see yet just like before the cache still had no updates.

For now I suppose I'll use update and cache.modify inside any mutations - which isn't ideal.

So, TLDR, the issues when using @client with normalization:

  1. cache.writeFragment or cache.modify inside a merge field policy do not work
  2. Returned objects from a merge field policy are not normalize if the field is local
  3. Return object from a read field policy are not normalized if the field is local
  4. optimisticResponse will trigger the Mutation graphSettingsUpdate merge field policy - but existingData is undefined, newData is just a reference, and cache.modify and cache.writeFragment do not seem to work

Versions

@apollo/client@3.3.12

@benjamn benjamn self-assigned this Apr 2, 2021
@benjamn
Copy link
Member

benjamn commented Apr 2, 2021

@wallzero Sorry for the complexity/frustration, but thank you for your patience in providing this level of detail.

A few ideas:

  • Can you try updating to @apollo/client@3.4.0-beta.19? I have a feeling Simplify EntityStore CacheGroup logic to improve InMemoryCache result caching. #7887 will have some (positive) impact here.
  • If you can put the problematic examples into a reproduction, or submit a PR adding some failing test cases (here's a test file with other optimistic update tests), then we can do a better job helping debug these problems.
  • I suspect the optimistic updates may be responsible for the written data disappearing, because the optimistic cache layers are removed after the mutation finishes. Omitting the optimisticResponse should make the cache behave in a more predictable / less complicated way (optimistic updates can be challenging to debug).
  • Do you have a policy for the GraphSettings type with keyFields: ["graphId"]? If not, it's possible the GraphSettings object isn't successfully identified by cache.identify, which could explain some of these problems.

@wallzero
Copy link
Author

wallzero commented Apr 2, 2021

@benjamn Thanks for getting back so quickly! With the holiday weekend approaching, I may not follow up until Monday/Tuesday - but here's a few notes.

  • After I made the post I tried updating to @apollo/client@3.4.0-beta.19. I ran into a few snags but now everything builds. I haven't had time to test all the different permutations but a couple quick tests show cache.modify and cache.writeFragment are still not pushing changes to the cache when run inside merge field policies. Also, if I remove the cache.writeFragment from the settings read field policy in the Graph policy, GraphSettings no longer normalize and the Graph cached graph doesn't have a settings field.
  • I'd like to start with making a reproduction next week; just in case it's something silly on my end.
  • Removing the optimisticResponse is not something I had considered. Just now I tried removing both optimisticResponse and update, and the mutation graphSettingsUpdate merge and Graph merge field policies are never called. Also nothing was sent to the cache.
  • Yes I have had a policy for GraphSettings:
NodsicGraphSettings: {
	keyFields: ['graphId']
}

So after reflecting for a bit I think it's easier to distill this into two complementing issues:

  1. I don't think read field responses (ref's or objects) are stored in the cache or normalized when the field query uses @client. In a query like:
query GraphPreload (
	$graphId: ID!
) {
	graph (
		graphId: $graphId
	) {
		graphId
		name
		settings @client {
			graphId
			zoom
			x
			y
		}
	}
}

...the settings read function in the Graph field policy will trigger, but the response is never cached or normalized.

  1. cache.modify and cache.writeFragment don't appear to work inside merge field policies.

I should be able to whip up a reproduction in a few days. Thanks again!

@wallzero
Copy link
Author

wallzero commented Apr 6, 2021

@benjamn Here is an early reproduction: https://github.com/wallzero/react-apollo-error-template

I was able to demonstrate the first issue; read field responses (ref's or objects) are not cached/normalized when the field query uses @client. writeQuery also does not cache/normalize fields that are under @client.

The second issue is actually working in the reproduction - cache.writeFragment is working inside merge functions. I'll keep trying to see why it works in the reproduction but fails in my project.

@wallzero
Copy link
Author

wallzero commented Apr 6, 2021

I'm pretty sure what is happening is cache.modify and cache.writeFragment are working inside the merge functions, but something causes the cache to revert. If I use optimisticResponse, my read field policies are called twice. The first time they are called I can see that cache.modify/cache.writeFragment has worked. The second time they are called the cache has been reverted somehow.

@benjamn benjamn added 🌹 has-reproduction and removed 🏓 awaiting-contributor-response requires input from a contributor labels Jun 1, 2021
@cesarve77
Copy link

Same Issue

@cesarve77
Copy link

any advance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants