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.

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

Ensure InMemoryCache#read{,Query,Fragment} always return T | null. #7098

Merged
merged 5 commits into from
Sep 30, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 29, 2020

Closes apollographql/apollo-feature-requests#1, by making cache.read no longer throw a MissingFieldError 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 a MissingFieldError, it will now return null instead. We believe this change will be backwards-compatible because null was already a possible return value, so existing code should be prepared to handle null. However, this hypothesis will need to be verified empirically, using real applications.

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 to allow 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 here describing your use case. We will let these changes bake in AC3.3 betas until we are confident they have been adequately tested.

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.
@benjamn benjamn added this to the Post 3.0 milestone Sep 29, 2020
@benjamn benjamn self-assigned this Sep 29, 2020
benjamn added a commit that referenced this pull request Sep 29, 2020
This change means the existence of root objects like ROOT_QUERY and
ROOT_MUTATION will no longer depend on whether other queries/mutations
have previously written data into the cache.

This matters because (until just recently) cache.read would return null
for a completely missing ROOT_QUERY object, but throw missing field errors
if ROOT_QUERY existed but did not have the requested fields.

In other words, this commit hides the difference between the absence of
ROOT_QUERY and its incompleteness, so unrelated cache writes can no longer
unexpectedly influence the null-returning vs. exception-throwing behavior
of cache.read.

As an additional motivation, with AC3 field policies, it's possible now to
define synthetic root query fields that have a chance of returning useful
values even if the cache is completely empty. Providing a default empty
object for root IDs like ROOT_QUERY is important to let those read
functions be called, instead of prematurely returning null from cache.read
when ROOT_QUERY does not exist.

Note: this PR builds on PR #7098.
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.

Awesome, looks great @benjamn - thanks!

benjamn added a commit that referenced this pull request Sep 30, 2020
This change means the existence of root objects like ROOT_QUERY and
ROOT_MUTATION will no longer depend on whether other queries/mutations
have previously written data into the cache.

This matters because (until just recently) cache.read would return null
for a completely missing ROOT_QUERY object, but throw missing field errors
if ROOT_QUERY existed but did not have the requested fields.

In other words, this commit hides the difference between the absence of
ROOT_QUERY and its incompleteness, so unrelated cache writes can no longer
unexpectedly influence the null-returning vs. exception-throwing behavior
of cache.read.

As an additional motivation, with AC3 field policies, it's possible now to
define synthetic root query fields that have a chance of returning useful
values even if the cache is completely empty. Providing a default empty
object for root IDs like ROOT_QUERY is important to let those read
functions be called, instead of prematurely returning null from cache.read
when ROOT_QUERY does not exist.

Note: this PR builds on PR #7098.
@benjamn benjamn merged commit 93f29c5 into release-3.3 Sep 30, 2020
@benjamn benjamn deleted the fr1-prevent-cache.read-from-throwing branch September 30, 2020 20:52
benjamn added a commit that referenced this pull request Sep 30, 2020
This change means the existence of root objects like ROOT_QUERY and
ROOT_MUTATION will no longer depend on whether other queries/mutations
have previously written data into the cache.

This matters because (until just recently) cache.read would return null
for a completely missing ROOT_QUERY object, but throw missing field errors
if ROOT_QUERY existed but did not have the requested fields.

In other words, this commit hides the difference between the absence of
ROOT_QUERY and its incompleteness, so unrelated cache writes can no longer
unexpectedly influence the null-returning vs. exception-throwing behavior
of cache.read.

As an additional motivation, with AC3 field policies, it's possible now to
define synthetic root query fields that have a chance of returning useful
values even if the cache is completely empty. Providing a default empty
object for root IDs like ROOT_QUERY is important to let those read
functions be called, instead of prematurely returning null from cache.read
when ROOT_QUERY does not exist.

Note: this PR builds on PR #7098.
benjamn added a commit that referenced this pull request Sep 30, 2020
This change means the existence of root objects like ROOT_QUERY and
ROOT_MUTATION will no longer depend on whether other queries/mutations
have previously written data into the cache.

This matters because (until just recently) cache.read would return null
for a completely missing ROOT_QUERY object, but throw missing field errors
if ROOT_QUERY existed but did not have the requested fields.

In other words, this commit hides the difference between the absence of
ROOT_QUERY and its incompleteness, so unrelated cache writes can no longer
unexpectedly influence the null-returning vs. exception-throwing behavior
of cache.read.

As an additional motivation, with AC3 field policies, it's possible now to
define synthetic root query fields that have a chance of returning useful
values even if the cache is completely empty. Providing a default empty
object for root IDs like ROOT_QUERY is important to let those read
functions be called, instead of prematurely returning null from cache.read
when ROOT_QUERY does not exist.

Note: this PR builds on PR #7098.
CarsonF added a commit to SeedCompany/cord-field that referenced this pull request Jan 21, 2021
CarsonF added a commit to SeedCompany/cord-field that referenced this pull request Jan 21, 2021
@chriswetterman
Copy link

@benjamn Thank you for providing the info about cache.diff. I was chasing an issue using optimisticResponse on a mutation to create an entity where I only have a couple fields the user creates but the response has a slew of additional data. Thanks to cache.diff I saw I was receiving 5 MissingFieldError's.

If I could ask, since the behavior of cache.readQuery has changed can the API docs be updated to include a portion on cache.diff and a reference made to utilize cache.diff for further information on why cache.readQuery may have returned null?

Thanks.

@josnin
Copy link

josnin commented Aug 7, 2021

is there an equivalent of cache.diff for readFragment to debug which is causing to return null?

@apollographql apollographql locked and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants