Skip to content
This repository has been archived by the owner on Jul 10, 2019. It is now read-only.

Add resolver checking to fix undefined bug and return defaults when no resolver present #202

Merged
merged 5 commits into from
Feb 12, 2018

Conversation

evans
Copy link
Contributor

@evans evans commented Feb 9, 2018

This fixes #198 #194 in their current form.

This PR adds a check for the presence of a resolverMap. The Queries: () => ({}) getaround worked detailed in this comment, since it created a value for resolverMap. For a client GraphQL queries that pulls directly from the cache(no resolver specified), a resetStore 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 of resetStore: empties store, calls broadcastQueries(notifies the link-state resolver), then calls the onResetStore callbacks(writes link-state defaults). We need to either reorder resetStore(a "breaking" change, given that the order is not documented) or add another function, such as onEmptyStore that will be called any time the Apollo Cache is empty.

@codecov-io
Copy link

codecov-io commented Feb 9, 2018

Codecov Report

Merging #202 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
packages/apollo-link-state/src/index.ts 97.01% <100%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 878ab42...5adbc5b. Read the comment docs.

Copy link
Contributor

@peggyrayzis peggyrayzis left a 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';
Copy link
Contributor

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?

@evans
Copy link
Contributor Author

evans commented Feb 12, 2018

I added to the changelog and removed the imports. Here is the docs PR

@peggyrayzis
Copy link
Contributor

Thanks for making those changes!! 🎉

@ghost
Copy link

ghost commented Mar 4, 2018

@peggyrayzis can you release 0.4.1 for this fixed problem? Thanks.

@gopeter
Copy link

gopeter commented Mar 6, 2018

This would be really great! :)

@fracmak
Copy link

fracmak commented Jun 20, 2018

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.

@danilobuerger
Copy link

@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 .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants