-
Notifications
You must be signed in to change notification settings - Fork 101
Add resolver checking to fix undefined bug and return defaults when no resolver present #202
Conversation
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
==========================================
+ Coverage 96.15% 96.25% +0.09%
==========================================
Files 2 2
Lines 78 80 +2
Branches 21 22 +1
==========================================
+ Hits 75 77 +2
Misses 3 3
Continue to review full report at Codecov.
|
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.
Thank you SO much for fixing this!! I'm really impressed that you were able to find a solution that didn't involve changes to the core.
Before we merge, I have one small question regarding imports. Could you please also add the PR to the changelog?
What do you think about adding a section in the docs that explain the limitations for resetting the store?
@@ -7,6 +7,20 @@ import { print } from 'graphql/language/printer'; | |||
import { parse } from 'graphql/language/parser'; | |||
|
|||
import { withClientState } from '../'; | |||
import { ApolloCache } from 'apollo-cache'; |
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.
Are we using this and the following two imports anywhere? If not, can we please remove them?
I added to the changelog and removed the imports. Here is the docs PR |
Thanks for making those changes!! 🎉 |
@peggyrayzis can you release 0.4.1 for this fixed problem? Thanks. |
This would be really great! :) |
I've found this causes a regression for me, might be related to #262. When you don't define a Query resolver, depending on how you craft your queries, the default value is sometimes returned instead of the current value in the cache. |
@evans @peggyrayzis This broke #262 because you decided to always fallback to returning the default which does not work when mixing remote / local as explained in #262 . |
This fixes #198 #194 in their current form.
This PR adds a check for the presence of a
resolverMap
. TheQueries: () => ({})
getaround worked detailed in this comment, since it created a value forresolverMap
. For a client GraphQL queries that pulls directly from the cache(no resolver specified), aresetStore
appears to cause no issues. A link-state Query resolver that returns a raw value is also fine.The unresolved issue is a Query resolver that attempts to pull from the cache after a
resetStore
. This is due to the ordering ofresetStore
: empties store, callsbroadcastQueries
(notifies the link-state resolver), then calls theonResetStore
callbacks(writes link-state defaults). We need to either reorderresetStore
(a "breaking" change, given that the order is not documented) or add another function, such asonEmptyStore
that will be called any time the Apollo Cache is empty.