Let InMemoryCache return null if cache is empty #5866
Closed
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
|
@jannes-io: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
|
Failing test because of missing field oversight. I'll investigate further to come to a nicer solution whilst keeping the missing field errors. |
This will allow the missing fields and dangling object errors to continue being thrown while other errors are caught and transformed to null like discussed in #1.
|
Tests are passing now. |
benjamn
changed the title
benjamn
requested changes
Jan 28, 2020
| try { | ||
| return this.cache.readQuery<T, TVariables>(options, optimistic); | ||
| } catch (e) { | ||
| return null; |
There was a problem hiding this comment.
A blind try-catch block that swallows all exceptions cannot the right way to do this. This repository controls the cache as well as the client, so there's no reason we should need a band-aid like this.
There was a problem hiding this comment.
I fully agree, unfortunately I am not familiar enough with the apollo code-base and just wanted to do a suggestion and raise some attention to the implementation of the requested feature as our code-base for a new project is currently suffering from it
| // separation is to include c.callback in the cache key for | ||
| // maybeBroadcastWatch calls. See issue #5733. | ||
| c.callback, | ||
| JSON.stringify({ optimistic, rootId, variables }), |
There was a problem hiding this comment.
Please revert these whitespace changes.
|
Thanks for the PR and thanks for reviewing it. Is this likely to be merged? |
|
@EmreErdogan no, this would indeed solve the issue, but is not a good way to go about it. Someone with more knowledge about the inMemoryCache will have to take a closer look and come up with a solid fix instead of the band-aid that is this PR. |
benjamn
added a commit
that referenced
this pull request
Feb 10, 2020
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
benjamn
added a commit
that referenced
this pull request
Feb 11, 2020
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
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.
#1 refers to the feature-request repository:
apollographql/apollo-feature-requests#1
Checklist: