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

Support inheritance of typePolicies, according to possibleTypes. #7065

Merged
merged 6 commits into from
Sep 24, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 23, 2020

Motivating discussion: #6949 (comment)

JavaScript developers will be familiar with the idea of inheritance from the extends clause of class declarations, or possibly from dealing with prototype chains created by Object.create.

Here's how type policy inheritance works for InMemoryCache:

const cache = new InMemoryCache({
  possibleTypes: {
    Reptile: ["Snake", "Turtle"],
    Snake: ["Python", "Viper", "Cobra"],
    Viper: ["Cottonmouth", "DeathAdder"],
  },

  typePolicies: {
    Reptile: {
      // Suppose all our reptiles are captive, and have a tag with an ID.
      keyFields: ["tagId"],
      fields: {
        // Scientific name-related logic can be shared among Reptile subtypes.
        scientificName: {
          merge(_, incoming) {
            // Normalize all scientific names to lower case.
            return incoming.toLowerCase();
          },
        },
      },
    },

    Snake: {
      fields: {
        // Default to a truthy non-boolean value if we don't know
        // whether this snake is venomous.
        venomous(status = "unknown") {
          return status;
        },
      },
    },
  },
});

Inheritance is a powerful code-sharing tool, and it works well for Apollo Client for several reasons:

  1. InMemoryCache already knows about the supertype-subtype relationships (interfaces and unions) in your schema, thanks to possibleTypes, so no additional configuration is necessary to provide that information.

  2. 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.

  3. A single subtype can have multiple supertypes in a GraphQL schema, which is difficult to model using the single inheritance model of classes 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.

  4. 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.

  5. 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 for possibleTypes, and should drastically reduce repetition of typePolicies when used effectively.

Inheritance is a step back (in terms of freedom/power) from the completely dynamic policyForType and policyForField functions I proposed in #6808 (comment), but I think inheritable typePolicies can address most of the concerns raised in #6808.

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.
@benjamn benjamn added this to the Post 3.0 milestone Sep 23, 2020
@benjamn benjamn self-assigned this Sep 23, 2020
@benjamn benjamn changed the title Type policies inheritance Support inheritance of typePolicies, according to possibleTypes. Sep 23, 2020
@vigie
Copy link

vigie commented Sep 24, 2020

Thanks for working on this. In your example, could you explain what would happen if Snake also declares a field policy for scientificName?

Copy link
Member

@hwillson hwillson left a 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?

@hwillson
Copy link
Member

@vigie subtype field policies override supertype field policies, with the same name. So a Snake scientificName field policy will override the Reptile scientificName field policy.

Comment on lines +466 to +494
// 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);
});
}
Copy link
Member Author

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.

Copy link

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?

@benjamn benjamn deleted the typePolicies-inheritance branch September 24, 2020 20:57
benjamn added a commit that referenced this pull request Dec 11, 2020
Thanks to @zaguiini for tracking this regression (#7443) back to my
type/field policy inheritance PR (#7065).
@jpvajda jpvajda mentioned this pull request Jul 15, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 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