-
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
Replace options.isReference(ref, true) with options.canRead(ref). #6425
Conversation
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) : []; |
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.
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.
return isReference(objOrRef) | ||
? this.has(objOrRef.__ref) | ||
: typeof objOrRef === "object"; |
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.
Intentionally allowing canRead(null)
to return true
, since null
is a valid placeholder value in GraphQL lists.
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 |
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.
Looks great @benjamn! Doing the filtering automatically definitely looks tempting ...
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.
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.
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
, andmodify
functions that determines if a given object can be read withoptions.readField
.It was tempting to call this helper function
options.isReadable
, butoptions.canRead
is shorter and less easily confused withoptions.isReference
(which matters for auto-completion and dyslexic programmers), sooptions.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-normalizedStoreObject
s:Using
isReference(friend, true)
here would have worked only ifisReference(friend)
wastrue
, whereascanRead(friend)
also returnstrue
when friend is a non-Reference
object with readable fields, which can happen if theFriend
type has no ID and/orkeyFields: false
.Now that we have
options.canRead
andoptions.readField
, I think the use cases foroptions.isReference
are probably pretty scarce, but I don't want to yankisReference
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 byoptions.canRead
, so we can decide how much documentation to give the various helpers.