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

[docs] Update cache-configuration keyFields docs #11778

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

smyrick
Copy link
Member

@smyrick smyrick commented Apr 10, 2024

The docs were missing an explanation of how the keyFields can use false or a function

I would also appreciate a review from the team as I wrote this up from TS types

The docs were missing an explanation of how the keyFields can use false or a function
@smyrick smyrick requested a review from a team as a code owner April 10, 2024 23:32
Copy link

changeset-bot bot commented Apr 10, 2024

⚠️ No Changeset found

Latest commit: 0c33efd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Apr 10, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 0c33efd
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/66305216b407320008bc124a
😎 Deploy Preview https://deploy-preview-11778--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate the contribution here! Having the keyFields function example is super helpful and not sure why we didn't have it before.

There are a few inaccuracies though that I'd love to correct before merging. Mind making those changes? I'd be happy to get this in afterwards.

docs/source/caching/cache-configuration.mdx Outdated Show resolved Hide resolved
docs/source/caching/cache-configuration.mdx Outdated Show resolved Hide resolved
@@ -194,7 +208,8 @@ This example shows a variety of `typePolicies` with different `keyFields`:
```
Book:{"title":"Fahrenheit 451","author":{"name":"Ray Bradbury"}}
```
* The `AllProducts` type illustrates a special strategy for a **singleton** type. If the cache will only ever contain one `AllProducts` object and that object has _no_ identifying fields, you can provide an empty array for its `keyFields`.
* The `AllProducts` type illustrates a special strategy for a **singleton** type. If the cache will only ever contain one `AllProducts` object and that object has _no_ identifying fields, you can provide an empty array or `false` for its `keyFields`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave this the same as it was before. Perhaps another bullet point stating that keyFields: false disables normalization? It doesn't have anything to do with singletons though.

docs/source/caching/cache-configuration.mdx Outdated Show resolved Hide resolved
Comment on lines 192 to 193
// Or we can indicate that this type is a Singleton
return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a keyFields function, returning false would indicate that this particular instance of a type shouldn't be normalized and instead embedded in the parent, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamesque thats correct! Here is a test that demonstrates returning false from a keyFields function. You can see the cache is non-normalized:

it(`allows keyFields and keyArgs functions to return false`, function () {
const cache = new InMemoryCache({
typePolicies: {
Person: {
keyFields() {
return false;
},
fields: {
height: {
keyArgs() {
return false;
},
merge(_, height, { args }) {
if (args) {
if (args.units === "feet") {
return height;
}
if (args.units === "meters") {
// Convert to feet so we don't have to remember the units.
return height * 3.28084;
}
}
throw new Error("unexpected units: " + args);
},
},
},
},
},
});
const query = gql`
query GetUser($units: string) {
people {
id
height(units: $units)
}
}
`;
cache.writeQuery({
query,
variables: {
units: "meters",
},
data: {
people: [
{
__typename: "Person",
id: 12345,
height: 1.75,
},
{
__typename: "Person",
id: 23456,
height: 2,
},
],
},
});
expect(cache.extract()).toEqual({
ROOT_QUERY: {
__typename: "Query",
// An array of non-normalized objects, not Reference objects.
people: [
{
__typename: "Person",
// No serialized units argument, just "height".
height: 5.74147,
id: 12345,
},
{
__typename: "Person",
height: 6.56168,
id: 23456,
},
],
},
});
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Million-dollar question — is that true for any falsy value? It seems like it based on this or this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think thats accurate, but I'd like to verify myself before giving a definitive yes. Intuitively it would make sense that if we don't get an identifier for a particular store object that we'd consider it non-normalized, but again, let me check to make sure thats accurate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamesque just tried this out and I can confirm it seems that any falsey value will be treated as false and won't normalize the record.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! I don't know exactly why ReturnType<IdGetter> is important to include in the return type union but it would definitely be clearer if it was just KeySpecifier | false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! I'll see if we can simplify that type.

Copy link
Member

@phryneas phryneas Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReturnType<IdGetter> does add undefined (which is unfortunate because we probably want people to be more explicit with false) and string, which is important - the keyFn function can not only return an array of field names, but also directly an identifier (although I'd advise against using that in many situations and leave the field-reading to the InMemoryCache instead).

I probably wouldn't change the type around to remove undefined here, since probably a lot of people just skip the return call in some conditions and that would break a lot of code bases for no very good reason.

@smyrick
Copy link
Member Author

smyrick commented Apr 13, 2024

@jerelmiller @adamesque I made some doc updates, but feel free to add inline suggestions or more context as needed

Comment on lines 189 to 197
// You can also use a function to determine any of the values above.
// The first argument is the reference to the record to be written, and the second is the runtime context
keyFields: (location, context) => {
if (location.state) {
return ["city, "state", "country"];
} else {
return ["city, "country"];
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this comment, I believe this should be context.readField("location") or context.storeObject.location and we should steer folks away from using the first arg altogether since it represents the raw server response which includes aliased fields and won't follow refs (aka instead of refs to other nested cache objects you'll only get the fields selected).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you mention this in a later bullet point, but my worry is that folks will read the code example and not the following notes and be surprised.

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

Successfully merging this pull request may close these issues.

4 participants