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.

Sign up for GitHub

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

Jump to bottom

Support options.isReference(ref, true) to check Reference validity. #6413

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Support options.isReference(ref, true) to check Reference validity. #6413

merged 1 commit into from
Jun 9, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 8, 2020 •
edited
Loading

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.

@benjamn benjamn added this to the Release 3.0 milestone Jun 8, 2020
@benjamn benjamn self-assigned this Jun 8, 2020
@benjamn benjamn requested a review from hwillson June 8, 2020 23:00
@benjamn benjamn marked this pull request as draft June 8, 2020 23:18
@benjamn

This comment has been minimized.

Sign in to view
@benjamn benjamn force-pushed the options.isReference-mustBeValid-arg branch 2 times, most recently from 4dce903 to 0aec5ea Compare June 8, 2020 23:55
@benjamn
Support options.isReference(ref, true) to check Reference validity.
a66d97c 
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.
@benjamn benjamn force-pushed the options.isReference-mustBeValid-arg branch from 0aec5ea to a66d97c Compare June 8, 2020 23:58
@benjamn benjamn marked this pull request as ready for review June 8, 2020 23:59
@benjamn
Copy link
Member Author

benjamn commented Jun 9, 2020

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 options.isReference(ref, true) is vital to our story about properly handling dangling references, and so its absence would feel like a bug.

hwillson
hwillson approved these changes Jun 9, 2020
View reviewed changes
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 @benjamn - makes sense! 👍

@benjamn benjamn merged commit 6f8d1b7 into master Jun 9, 2020
@benjamn benjamn deleted the options.isReference-mustBeValid-arg branch June 9, 2020 14:24
@benjamn benjamn mentioned this pull request Jun 9, 2020
Closed
benjamn added a commit that referenced this pull request Jun 10, 2020
@benjamn
Replace options.isReference(ref, true) with options.canRead(ref).
9ae57f1 
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 benjamn mentioned this pull request Jun 10, 2020
Merged
benjamn added a commit that referenced this pull request Jun 10, 2020
@benjamn
Replace options.isReference(ref, true) with options.canRead(ref).
f25ae9f 
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
@benjamn
Replace options.isReference(ref, true) with options.canRead(ref). (#6425
07d89c4 
)

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.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hwillson hwillson hwillson approved these changes

Assignees

@benjamn benjamn

Labels
🧞‍♂️ enhancement 🤔 needs-rethinking
Projects
None yet
Milestone
Release 3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@benjamn @hwillson