Support options.isReference(ref, true) to check Reference validity.#6413
Merged
Support options.isReference(ref, true) to check Reference validity.#6413
Conversation
This comment has been minimized.
This comment has been minimized.
4dce903 to
0aec5ea
Compare
The story we're telling in #6412 about using custom read functions to filter out dangling references works best if there's an easy way to check whether a Reference points to existing data in the cache. Although we could have introduced a new options.isValidReference helper function, I think it makes sense to let the existing options.isReference function handle this use case as well.
0aec5ea to
a66d97c
Compare
Member
Author
|
I know this may seem like a new feature, and we said we're only fixing bugs during the RC testing phase, but I think |
benjamn
added a commit
that referenced
this pull request
Jun 10, 2020
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
added a commit
that referenced
this pull request
Jun 10, 2020
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
added a commit
that referenced
this pull request
Jun 10, 2020
) 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The story we're telling in #6412 about using custom
readfunctions to filter out dangling references works best if there's an easy way to check whether aReferencepoints to existing data in the cache. Although we could have introduced a newoptions.isValidReferencehelper function, I think it makes sense to let the existingoptions.isReferencefunction handle this use case as well.