Replace options.isReference(ref, true) with options.canRead(ref). by benjamn · Pull Request #6425 · apollographql/apollo-client

benjamn · 2020-06-10T18:49:18Z

After thinking more about PR #6413, which has been merged but not yet released, I think it makes more sense to have a separate helper function for read, merge, and modify functions that determines if a given object can be read with options.readField.

It was tempting to call this helper function options.isReadable, but options.canRead is shorter and less easily confused with options.isReference (which matters for auto-completion and dyslexic programmers), so options.canRead is what I settled on.

This new helper makes it easy to filter out dangling references from lists of data correctly, without having to know if the list elements are normalized Reference objects, or non-normalized StoreObjects:

new InMemoryCache({
  typePolicies: {
    Person: {
      fields: {
        friends(existing, { canRead }) {
          return existing ? existing.filter(canRead) : [];
        },
      },
    },
  },
})

Using isReference(friend, true) here would have worked only if isReference(friend) was true, whereas canRead(friend) also returns true when friend is a non-Reference object with readable fields, which can happen if the Friend type has no ID and/or keyFields: false.

Now that we have options.canRead and options.readField, I think the use cases for options.isReference are probably pretty scarce, but I don't want to yank isReference away from folks who have been using it in the betas/RCs.

Please let us know if you have a specific use case for options.isReference that is not covered by options.canRead, so we can decide how much documentation to give the various helpers.

benjamn · 2020-06-10T18:51:20Z

src/cache/inmemory/__tests__/readFromStore.ts

-            children(offspring: Reference[], { isReference }) {
-              return offspring ? offspring.filter(
-                // The true argument here makes isReference return true
-                // only if child is a Reference object that points to
-                // valid entity data in the EntityStore (that is, not a
-                // dangling reference).
-                child => isReference(child, true)
-              ) : [];
+            children(offspring: Reference[], { canRead }) {
+              // Automatically filter out any dangling references, and
+              // supply a default empty array if !offspring.
+              return offspring ? offspring.filter(canRead) : [];


This seems like a clear improvement, to me.

After thinking more about PR #6413, which has been merged but not yet released, I think it makes more sense to have a separate helper function for read, merge, and modify functions that determines if a given object can be read with options.readField. It was tempting to call this helper function options.isReadable, but options.canRead is shorter and less easily confused with options.isReference (which matters for auto-completion and dyslexic programmers), so options.canRead is what I settled on. This new helper makes it easy to filter out dangling references from lists of data correctly, without having to know if the list elements are normalized Reference objects, or non-normalized StoreObjects: new InMemoryCache({ typePolicies: { Person: { fields: { friends(existing, { canRead }) { return existing ? existing.filter(canRead) : []; }, }, }, }, }) Using isReference(friend, true) here would have worked only if isReference(friend) was true, whereas canRead(friend) also returns true when friend is a non-Reference object with readable fields, which can happen if the Friend type has no ID and/or keyFields:false. Now that we have options.canRead and options.readField, I think the use cases for options.isReference are probably pretty scarce, but I don't want to yank isReference away from folks who have been using it in the betas/RCs. Please let us know if you have a specific use case for options.isReference that is not covered by options.canRead, so we can decide how much documentation to give the various helpers.

benjamn · 2020-06-10T19:04:03Z

src/cache/inmemory/entityStore.ts

+    return isReference(objOrRef)
+      ? this.has(objOrRef.__ref)
+      : typeof objOrRef === "object";


Intentionally allowing canRead(null) to return true, since null is a valid placeholder value in GraphQL lists.

benjamn · 2020-06-10T19:06:00Z

If we wanted to do this filtering automatically, I think this is the only change we need:

commit 6fe8075f53ae33ce28a0aecc1b2e888747b2d166
Author: Ben Newman <[email protected]>
Date:   Wed Jun 10 15:04:55 2020 -0400

    Automatically filter out dangling references from arrays.

diff --git a/src/cache/inmemory/readFromStore.ts b/src/cache/inmemory/readFromStore.ts
index 4143f72d..a0a0687b 100644
--- a/src/cache/inmemory/readFromStore.ts
+++ b/src/cache/inmemory/readFromStore.ts
@@ -377,40 +377,44 @@ export class StoreReader {
 
   // Uncached version of executeSubSelectedArray.
   private execSubSelectedArrayImpl({
     field,
     array,
     context,
   }: ExecSubSelectedArrayOptions): ExecResult {
     let missing: MissingFieldError[] | undefined;
 
     function handleMissing<T>(childResult: ExecResult<T>, i: number): T {
       if (childResult.missing) {
         missing = missing || [];
         missing.push(...childResult.missing);
       }
 
       invariant(context.path.pop() === i);
 
       return childResult.result;
     }
 
+    if (field.selectionSet) {
+      array = array.filter(context.store.canRead);
+    }
+
     array = array.map((item, i) => {
       // null value in array
       if (item === null) {
         return null;
       }
 
       context.path.push(i);
 
       // This is a nested array, recurse
       if (Array.isArray(item)) {
         return handleMissing(this.executeSubSelectedArray({
           field,
           array: item,
           context,
         }), i);
       }
 
       // This is an object, run the selection set on it
       if (field.selectionSet) {
         return handleMissing(this.executeSelectionSet({

With this change, you would not need to define a read function for Person.friends in the example above (except to get the default []).

hwillson

Looks great @benjamn! Doing the filtering automatically definitely looks tempting ...

jcreighton

I love how much clearer this is!

This commit implements the proposal for automatic filtering of dangling references that I described in #6425 (comment). The filtering functionality demonstrated by #6412 (and updated by #6425) seems useful enough that we might as well make it the default behavior for any array-valued field consumed by a selection set. Note: the presence of field.selectionSet implies the author of the query expects the elements to be objects (or references) rather than scalar values. By making .filter(canRead) automatic, we free developers from having to worry about manually removing any references after evicting entities from the cache. Instead, those dangling references will simply (appear to) disappear from cache results, which is almost always the desired behavior. In case this automatic filtering is not desired, a custom read function can be used to override the filtering, since read functions run before this filtering happens. This commit includes tests demonstrating several options for replacing/filtering dangling references in non-default ways.

This commit implements the proposal for automatic filtering of dangling references that I described in #6425 (comment). The filtering functionality demonstrated by #6412 (and updated by #6425) seems useful enough that we might as well make it the default behavior for any array-valued field consumed by a selection set. Note: the presence of field.selectionSet implies the author of the query expects the elements to be objects (or references) rather than scalar values. A list of scalar values should not be filtered, since it cannot contain dangling references. By making .filter(canRead) automatic, we free developers from having to worry about manually removing any references after evicting entities from the cache. Instead, those dangling references will simply (appear to) disappear from cache results, which is almost always the desired behavior. Fields whose values hold single (non-list) dangling references cannot be automatically filtered in the same way, but you can always write a custom read function for the field, and it's somewhat more likely that a refetch will fix those fields correctly. In case this automatic filtering is not desired, a custom read function can be used to override the filtering, since read functions run before this filtering happens. This commit includes tests demonstrating several options for replacing/filtering dangling references in non-default ways.

benjamn added 💡 idea 🧞‍♂️ enhancement labels Jun 10, 2020

benjamn added this to the Release 3.0 milestone Jun 10, 2020

benjamn self-assigned this Jun 10, 2020

benjamn commented Jun 10, 2020

View reviewed changes

benjamn requested review from hwillson and jcreighton June 10, 2020 18:52

benjamn force-pushed the options.canRead branch from 9ae57f1 to f25ae9f Compare June 10, 2020 19:02

benjamn commented Jun 10, 2020

View reviewed changes

hwillson approved these changes Jun 10, 2020

View reviewed changes

jcreighton approved these changes Jun 10, 2020

View reviewed changes

benjamn merged commit 07d89c4 into master Jun 10, 2020

benjamn deleted the options.canRead branch June 10, 2020 20:27

benjamn mentioned this pull request Jun 17, 2020

Test that cache evictions propagate to parent queries. #6412

Merged

benjamn mentioned this pull request Jun 17, 2020

Automatically filter out dangling references from arrays. #6454

Merged

This was referenced Jul 10, 2020

readQuery after cache evict #6325

Closed

Query doesn't return empty entries from cached array (offset pagination) any more #6628

Open

benjamn mentioned this pull request Aug 17, 2020

TypePolicies read returning nonexistent reference returns {} instead of query result #6844

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace options.isReference(ref, true) with options.canRead(ref).#6425

Replace options.isReference(ref, true) with options.canRead(ref).#6425
benjamn merged 1 commit intomasterfrom
options.canRead

benjamn commented Jun 10, 2020 •

edited

Loading

Uh oh!

benjamn Jun 10, 2020

Uh oh!

benjamn Jun 10, 2020

Uh oh!

benjamn commented Jun 10, 2020 •

edited

Loading

Uh oh!

hwillson left a comment

Uh oh!

jcreighton left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

benjamn commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamn Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

benjamn Jun 10, 2020

Choose a reason for hiding this comment

Uh oh!

benjamn commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hwillson left a comment

Choose a reason for hiding this comment

Uh oh!

jcreighton left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjamn commented Jun 10, 2020 •

edited

Loading

benjamn commented Jun 10, 2020 •

edited

Loading