-
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
Allow cache.{read,write}Query to take options.id. #5930
Merged
benjamn
merged 3 commits into
master
from
allow-readQuery-and-writeQuery-to-take-options.id
Feb 11, 2020
Merged
Allow cache.{read,write}Query to take options.id. #5930
benjamn
merged 3 commits into
master
from
allow-readQuery-and-writeQuery-to-take-options.id
Feb 11, 2020
+36
−43
Conversation
This file contains 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
Writing data to a specific entity object should not require the additional boilerplate of creating a named fragment with a type condition. All the cache really cares about is the top-level selection set of the query or fragment used to read or write data, and from that perspective queries are an almost perfect substitute for fragments, in almost all cases. In other words, this code cache.writeFragment({ id, fragment: gql` fragment UnnecessaryFragmentName on UnnecessaryType { firstName lastName } `, data: { __typename: "UnnecessaryType", firstName, lastName, }, }); can now be written as just cache.writeQuery({ id, query: gql`{ firstName lastName }`, data: { firstName, lastName }, }); Once you get used to using queries instead of fragments, you may begin to wonder why you ever thought fragments were a good idea. To save you some existential turmoil: fragments are still a good idea if you really need their type condition behavior (that is, if you want the fragment to match only when the `on SomeType` condition holds), but otherwise passing options.id and options.query to cache.readQuery or cache.writeQuery is a lot simpler and just as powerful. If you need further convincing, the cache.readFragment and cache.writeFragment methods actually convert the given options.fragment document to a query document (using getFragmentQueryDocument) which includes the given options.fragment as a ...field. You can skip that needless conversion by just providing options.query to readQuery or writeQuery, and reserving readFragment and writeFragment for the rare cases where fragments actually have advantages over queries. We are not removing the cache.readFragment and cache.writeFragment methods at this time, though we may consider deprecating them in the future. As a side benefit, these changes happen to solve our longest outstanding feature request, apollographql/apollo-feature-requests#1, which recently inspired #5866. That's because options.rootId is now always defined when we do this check: https://github.com/apollographql/apollo-client/blob/9cd5e8f7552db65437b5e59b605268a336977e45/src/cache/inmemory/inMemoryCache.ts#L130-L134
hwillson
approved these changes
Feb 11, 2020
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 ❤️ this @benjamn - looks great, thanks!
This comment has been minimized.
This comment has been minimized.
benjamn
added a commit
that referenced
this pull request
Sep 29, 2020
Closes apollographql/apollo-feature-requests#1, by making cache.read no longer throw under any circumstances. This is a long-standing feature request that I partially addressed in #5930, but this commit goes all the way. Since the current behavior (sometimes returning T, sometimes null, and sometimes throwing) has been in place for so long, we do not make this change lightly, and we should state precisely what is changing: in every case where cache.read would previously throw an exception, it will now return null instead. Since these null results have the effect of hiding which fields were missing, you may wish to switch from cache.readQuery to cache.diff to gain more insight into why cache.read returned null. In fact, cache.diff (along with cache.watch) are the primary ApolloCache methods that Apollo Client uses internally, because the cache.diff API provides so much more useful information than cache.read provides. If you would prefer for cache.read to return partial data rather than null, you can now pass returnPartialData:true to cache.readQuery and cache.readFragment, though the default must remain false instead of true, for the reasons explained in the code comments. In the positive column, null should be easier to handle in update functions that use the readQuery/writeQuery pattern for updating the cache. Not only can you avoid wrapping cache.readQuery with a try-catch block, but you can safely spread ...null into an object literal, which is often something that happens between readQuery and writeQuery. If you were relying on the exception-throwing behavior, and you have application code that is not prepared to handle null, please leave a comment describing your use case. We will let these changes bake in AC3.3 betas until we are confident they have been adequately tested.
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.
Writing data to a specific entity object in the cache should not require the additional boilerplate of creating a named fragment with a type condition.
All the cache really cares about is the top-level selection set of the query or fragment used to read or write data, and from that perspective queries are an almost perfect substitute for fragments, in almost all cases.
In other words, this code
can now be written as just
Once you get used to using queries instead of fragments, you may begin to wonder why you ever thought fragments were a good idea. To save you some existential turmoil: fragments are still a good idea if you really need their type condition behavior (that is, if you want the fragment to match only when the
on SomeType
condition holds), but otherwise passingoptions.id
andoptions.query
tocache.readQuery
orcache.writeQuery
is a lot simpler and just as powerful.If you need further convincing, the
cache.readFragment
andcache.writeFragment
methods actually convert the givenoptions.fragment
document to a query document (usinggetFragmentQueryDocument
) which includes the givenoptions.fragment
as a...field
. You can skip that needless conversion by just providingoptions.query
toreadQuery
orwriteQuery
, and reservingreadFragment
andwriteFragment
for the rare cases where fragments actually have advantages over queries.We are not removing the
cache.readFragment
andcache.writeFragment
methods at this time, though we may consider deprecating them in the future.As a side benefit, these changes happen to solve our longest outstanding feature request, apollographql/apollo-feature-requests#1, which recently inspired #5866. That's because
options.rootId
is now always defined when we do this check.