-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support inheritance of typePolicies, according to possibleTypes. #7065
Conversation
There's no harm in returning an empty TypePolicy object for an unrecognized type name, so we can simplify this code by removing the option to prevent initial policy creation. Future commits will allow type policies to be inherited according to possibleTypes, which means an intermediate subtype can have inherited policies even if it does not have its own entry in typePolicies. It will be simpler to implement that behavior if we always initialize the policy the first time we call getTypePolicy for a particular type.
Passing typePolicies to the InMemoryCache constructor is something that typically happens during application startup, so it's important that the processing of typePolicies does not become a performance problem. Fortunately, we don't have to do all the processing of typePolicies until we actually look up a given type policy for the first time, which might occur much later in the lifetime of the application for some types. Once we implement inheritance of typePolicies, there will often be supertype-subtype relationships among the types listed in typePolicies. Without the laziness introduced by this commit, it would be necessary either to keep typePolicies in supertypes-before-subtypes (topological) order, or for addTypePolicies somehow to sort the types topologically. Fortunately, that careful ordering/sorting becomes completely unnecessary thanks to the laziness, because none of the types can be used until all of them have been registered.
Thanks for working on this. In your example, could you explain what would happen if Snake also declares a field policy for scientificName? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, in the end this turned out to be a surprisingly small amount of code changes, thanks to the policy internals. This looks awesome! Your PR description is also incredibly helpful - I think it could be added to the docs almost exactly as it is?
@vigie subtype field policies override supertype field policies, with the same name. So a |
22a832a
to
ff21785
Compare
// When the TypePolicy for typename is first accessed, instead of | ||
// starting with an empty policy object, inherit any properties or | ||
// fields from the type policies of the supertypes of typename. | ||
// | ||
// Any properties or fields defined explicitly within the TypePolicy | ||
// for typename will take precedence, and if there are multiple | ||
// supertypes, the properties of policies whose types were added | ||
// later via addPossibleTypes will take precedence over those of | ||
// earlier supertypes. TODO Perhaps we should warn about these | ||
// conflicts in development, and recommend defining the property | ||
// explicitly in the subtype policy? | ||
// | ||
// Field policy inheritance is atomic/shallow: you can't inherit a | ||
// field policy and then override just its read function, since read | ||
// and merge functions often need to cooperate, so changing only one | ||
// of them would be a recipe for inconsistency. | ||
// | ||
// Once the TypePolicy for typename has been accessed, its | ||
// properties can still be updated directly using addTypePolicies, | ||
// but future changes to supertype policies will not be reflected in | ||
// this policy, because this code runs at most once per typename. | ||
const supertypes = this.supertypeMap.get(typename); | ||
if (supertypes && supertypes.size) { | ||
supertypes.forEach(supertype => { | ||
const { fields, ...rest } = this.getTypePolicy(supertype); | ||
Object.assign(policy, rest); | ||
Object.assign(policy.fields, fields); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment about policy precedence and the handling of inherited field policies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so if I'm reading this correctly descendant definitions just clobber their ancestor definitions. That's clear and simple.
I'm curious whether you considered running the definitions in sequence, passing the result of each as the input to the next, kind of like the way a constructor chain would work in OO?
fefad86
to
d372cac
Compare
Motivating discussion: #6949 (comment)
JavaScript developers will be familiar with the idea of inheritance from the
extends
clause ofclass
declarations, or possibly from dealing with prototype chains created byObject.create
.Here's how type policy inheritance works for
InMemoryCache
:Inheritance is a powerful code-sharing tool, and it works well for Apollo Client for several reasons:
InMemoryCache
already knows about the supertype-subtype relationships (interfaces and unions) in your schema, thanks topossibleTypes
, so no additional configuration is necessary to provide that information.Inheritance allows a supertype to provide default configuration values to all its subtypes, including
keyFields
and individual field policies, which can be selectively overridden by subtypes that want something different.A single subtype can have multiple supertypes in a GraphQL schema, which is difficult to model using the single inheritance model of
class
es or prototypes. In other words, supporting multiple inheritance in JavaScript requires building a system something like this one, rather than just reusing built-in language features.Developers can add their own client-only supertypes to the
possibleTypes
map, as a way of reusing behavior across types, even if their schema knows nothing about those made-up supertypes.The
possibleTypes
map is currently used only for fragment matching purposes, which is an important but fairly small part of what the client does. Inheritance adds another compelling use forpossibleTypes
, and should drastically reduce repetition oftypePolicies
when used effectively.Inheritance is a step back (in terms of freedom/power) from the completely dynamic
policyForType
andpolicyForField
functions I proposed in #6808 (comment), but I think inheritabletypePolicies
can address most of the concerns raised in #6808.