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

Fix type policy inheritance involving fuzzy possibleTypes #10633

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Mar 8, 2023

In #10599 I suggested two typical/supported ways of configuring entity identification: keyFields: [...] (preferred, __typename-specific) and defining a dataIdFromObject function (legacy, catch-all).

Unless you can enforce a blanket assumption that all your types must have a single id field (unlikely, brittle), both approaches ultimately require listing the __typenames of all the types whose keyFields you want to configure, which can be annoying if your application uses many different entity types.

The good news: I think I've found a way to combine a few different InMemoryCache features to achieve a blanket keyFields policy without enumerating all the relevant __typename strings. This approach combines the following features:

With these two existing features combined, EntitySupertype can define keyFields: ["uid"] just once (using uid instead of id here to make sure we aren't accidentally relying on default id behavior), and then any __typename that ends with Entity should automatically inherit that same keyFields configuration, even if the type has no other type/field policies defined.

Note that EntitySupertype is a made-up client-side type, and does not necessarily need to exist in your schema, though if you already have a common supertype that would work (like Node), I would recommend reusing that existing type instead of making up a new one.

Picking ".*Entity" as a naming convention is obviously an opinionated choice, not appropriate for all schemas and types, but you can use several different patterns, as well as exact __typename strings, so hopefully there's enough flexibility here to make EntitySupertype cover all/most of your entity types.

The bad news: this story was slightly broken, since it implicitly required subtypes to have some type/field policy configuration in order to be capable of inheriting from EntitySupertype. You could go through and add empty type policies for the relevant subtypes, but that brings back the chore of manually enumerating all relevant __typename strings.

Neither of these InMemoryCache features is new, and both are definitely on the obscure side, but it's important to me to keep all existing features working as intended (unless we decide to remove them in a major version), no matter how obscure they may be.

@benjamn benjamn self-assigned this Mar 8, 2023
@changeset-bot

This comment was marked as resolved.

Comment on lines +631 to +653
const cache = new InMemoryCache({
possibleTypes: {
EntitySupertype: [".*Entity"],
},
typePolicies: {
Query: {
fields: {
coworkers: {
merge(existing, incoming) {
return existing ? existing.concat(incoming) : incoming;
},
},
},
},

// The point of this test is to ensure keyFields: ["uid"] can be
// registered for all __typename strings matching the RegExp /.*Entity/,
// without manually enumerating all of them.
EntitySupertype: {
keyFields: ["uid"],
},
},
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Although this may still look like a lot of boilerplate, the key point is that we don't need any mention of subtypes like CoworkerEntity or ManagerEntity in this configuration code.

data: {
coworkers: [
{ __typename: "CoworkerEntity", uid: "qwer", name: "Alessia" },
{ __typename: "CoworkerEntity", uid: "asdf", name: "Jerel" },
Copy link
Member

Choose a reason for hiding this comment

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

I'm honored to make it into a test 😆

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.

TIL you can use fuzzy type names for possibleTypes. I'd love if we could add some documentation around that behavior at some point.

Nice work!

src/cache/inmemory/__tests__/policies.ts Show resolved Hide resolved
@benjamn benjamn force-pushed the fix-typePolicies-inheritance-involving-fuzzy-possibleTypes branch from 777623f to f9ab7a2 Compare March 8, 2023 22:19
@phryneas
Copy link
Member

phryneas commented Mar 9, 2023

This looks like a very useful pattern - we should definitely get that into the docs!

@benjamn benjamn merged commit 90a06ee into release-3.8 Mar 9, 2023
@benjamn benjamn deleted the fix-typePolicies-inheritance-involving-fuzzy-possibleTypes branch March 9, 2023 19:04
@alessbell
Copy link
Member

Very cool! +1 on getting that pattern into the docs, created an issue here: #10639

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants