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

Allow @directive and $variable strings in field policy keyArgs: ["arg", "@dir", "$var"] arrays #8678

Merged

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 20, 2021

This PR provides a much better solution than the three I proposed in #8659 (comment) to the problem of including more than just arguments in the keyArgs: [...] array notation in field policies.

While nothing is changing in the way existing keyArgs: [...] configurations work, this PR will allow you to refer to directives like the @connection directive, or query variables, in the same array where you usually specify your keyArgs:

new InMemoryCache({
  typePolicies: {
    Query: {
      fields: {
        feed: {
          // Note: if you're bothered by the use of keyArgs for items that are not
          // field arguments (that is, directives and variables), keep reading!
          keyArgs: ["type", "@connection", ["key"], "$var"],
        },
      },
    },
  },
})

Information about directives is included by default in the field key if you don't provide a keyArgs: [...] configuration, but is lost when switching to keyArgs: [...], which can be surprising. You can alternatively provide a keyArgs(args, context) {...} function to return any key you like, taking context.field.directives into account, but that's a lot more involved and error-prone.

The cache configuration shown above allows queries to use @connection(key: ...) to provide additional identifying information for the Query.feed field, which is especially useful when more than one Query.feed query is using the same arguments for that field, but you want to keep the query results separate in the cache:

query FeedQuery($key: String = "main") {
  feed(type: $feedType) @connection(key: $key) {
    id
    author { name }
    text
  }
}

Within the cache, you'll now see field keys like the following:

expect(cache.extract()).toEqual({
  ROOT_QUERY: {
    __typename: "Query",
    'feed:{"type":"public","@connection":{"key":"main"}}': { __typename, id, author, text },
  },
})

While the @connection directive is a common and widely recommended way to solve this problem, I'm proud to say this implementation does not hard-code support for the @connection directive. You can name the directive anything you want, and it will serve the same purpose!

The "@connection", ["key"] syntax is a way to select only the key argument from the @connection(key: ..., ignored: ...) directive in the query, which in this case is determined by the $key variable, which defaults to "main". If you leave out the ["key"] part, all arguments passed to the directive will be included. I recommend locking things down to just the arguments you're expecting.

The $var syntax seems useful when you want the field's storage key to depend on a variable that's not otherwise passed to that field, perhaps because your server expects the variable to be passed as an argument to a different field than the one you're trying to configure.

Both @directive and $variable notations are unambiguous with actual arguments (and with each other), because field argument names can only start with an alphabetical letter or an underscore, according to the spec here and here.

The same tolerance we introduced for optional keyArgs arguments in #7109 applies to directives and variables, too: when there's no @connection directive on the Query.feed field in the query, or no $var variable provided by the query (true in this case), those details will simply be omitted from the field key, rather than triggering an error. Note: keyFields: [...] still throws an exception for missing primary keys.

One aesthetic problem with this approach is that the keyArgs configuration may seem misnamed, since it's handling more than just field arguments. In case that bothers you like it bothers me, I introduced a more general field key configuration that's synonymous in every way with keyArgs (except that key takes precedence if both are provided). I think this new name is the "right" name, since the goal of keyArgs/key is to come up with a uniquely identifying storage key for the field, not necessarily limited to the provided arguments. This insight allows us to avoid introducing new configurations like keyDirectives or keyVariables to accompany keyArgs, as I considered yesterday in #8659 (comment). Update: I reverted this part, since key didn't justify itself IMHO: #8659 (comment)

Issues potentially resolved by this PR:

Comment on lines -1060 to +1087
function computeKeyObject(
response: Record<string, any>,
function computeKeyFieldsObject(
Copy link
Member Author

Choose a reason for hiding this comment

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

We used to (ab)use computeKeyObject for both keyFields: [...] and keyArgs: [...], but this PR pushed me over the edge to separating them into two separate functions: computeKeyFieldsObject and computeFieldKeyObject.

Comment on lines +1157 to +1160
// TODO Cache this work somehow, a la aliasMap?
const directiveName = key.slice(1);
// If the directive appears multiple times, only the first
// occurrence's arguments will be used. TODO Allow repetition?
Copy link
Member Author

Choose a reason for hiding this comment

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

Some TODOs to consider.

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.

Thanks for working on this @benjamn! Adding some docs to https://www.apollographql.com/docs/react/caching/advanced-topics/ would be super useful (but we can always track this separately if you'd like to get this PR in place sooner than later to cut a new 3.5 beta).

@benjamn benjamn force-pushed the allow-directives-and-variables-in-field-key-policy branch from adc3b73 to 0c4c008 Compare August 25, 2021 18:36
@benjamn benjamn requested a review from StephenBarlow as a code owner August 25, 2021 19:33
benjamn added a commit that referenced this pull request Aug 25, 2021
@benjamn
Copy link
Member Author

benjamn commented Aug 25, 2021

@StephenBarlow Can you take a look at the changes from 573a8ea and let me know what you want to do with the other section about @connection directive? We should probably consolidate or at least link them together?

Note: I will probably merge this PR soon, and welcome feedback/edits in a follow-up PR.

@benjamn benjamn force-pushed the allow-directives-and-variables-in-field-key-policy branch from b488975 to 5362b19 Compare August 25, 2021 19:59
@benjamn benjamn merged commit 0f6e613 into release-3.5 Aug 25, 2021
@benjamn benjamn deleted the allow-directives-and-variables-in-field-key-policy branch August 25, 2021 21:37
@dir
Copy link

dir commented Aug 25, 2021

Oh hey, that's me

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
@benjamn benjamn changed the title Allow @directive and $variable strings in field policy key: ["arg", "@dir", "$var"] arrays Allow @directive and $variable strings in field policy keyArgs: ["arg", "@dir", "$var"] arrays Apr 17, 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.

3 participants