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

[#3030] Fix no-cache fetchPolicy returns null or undefined #3102

Merged
merged 9 commits into from Mar 23, 2018

Conversation

Projects
None yet
10 participants
@kamranayub
Contributor

kamranayub commented Mar 1, 2018

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

Fixes #3030

The issue

When using the no-cache fetch policy, Apollo was returning null or undefined data.

The cause

When the no-cache fetch policy is used, QueryManager does not write back to the data store. However, when reading results, the store is used as a fallback when newData is empty or observer.getLastResult() is empty inside a QueryListener.

Changes

Inside QueryManager.fetchRequest:

  • If no-cache fetch policy is used, we set the query newData directly with { result, complete } structure
  • The resultFromStore was not being set if no-cache was being used, so it now uses result.data

The query listener will then be passed this during broadcastQueries with the newData parameter.

Inside ObservableQuery.refetch:

  • All refetch queries were being forced to use network-only which means even if you intended to use no-cache as your fetch policy, on refetch results were still being written to the cache. Now it treats both network-only and no-cache the same. I almost wonder if it should include cache-and-network.
@meteor-bot

This comment has been minimized.

meteor-bot commented Mar 1, 2018

Messages
📖

Please add your name and email to the AUTHORS file (optional)

Generated by 🚫 dangerJS

@kamranayub

This comment has been minimized.

Contributor

kamranayub commented Mar 1, 2018

I've tested these changes locally and using no-cache works now! 💯 It also fixes the performance issue we were seeing (maybe I'll open a separate issue for that, it's not necessarily a "bug" but more due to the way we're using Apollo). When using the cache, we saw 1-3 second processing when marking query results due to the diffing. Now it's <100ms bypassing the cache.

@kamranayub

This comment has been minimized.

Contributor

kamranayub commented Mar 1, 2018

I have a related fix that I think needs to be split in two.

For some fetch policies, loading is not reported as expected when using query or watchQuery sometimes, see #2294

I noticed what seems to happen is that query listener will call observer.next() with loading: true for some fetch policies, like cache-and-network. This is expected but what's not expected is that the query() Promise resolves with loading: true with no data! I think the fix might be to prevent resolving the promise until loading is false. This works as expected with my no-cache fixes, but I don't want to include it in this PR--it probably needs two separate PRs; one for adding no-cache support to shouldNotifyIfLoading and one to prevent resolving the Promise if loading is true.

kamranayub@05b0f90

@@ -3,17 +3,16 @@
"configurations": [

This comment has been minimized.

@kamranayub

kamranayub Mar 1, 2018

Contributor

I can exclude this change if requested; but this fixes debugging tests in VS Code (I could create separate configs per package)

@@ -1069,6 +1069,10 @@ export class QueryManager<TStore> {
reject(e);
return;
}
} else {
this.setQuery(queryId, () => ({
newData: { result: result.data, complete: true },

This comment has been minimized.

@kamranayub

kamranayub Mar 1, 2018

Contributor

I am not well-informed enough to say if this is enough structure so looking for feedback here

This comment has been minimized.

@jbaxleyiii

jbaxleyiii Mar 23, 2018

Member

I think this is a great solution honestly. Its a tricky one for sure with how react-apollo uses the cache directly so I think improvements can be made there as well. But great stuff!

@@ -264,9 +265,13 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> {
}
// Override fetchPolicy for this call only
// only network-only and no-cache are safe to use
const isNetworkFetchPolicy =
fetchPolicy === 'network-only' || fetchPolicy === 'no-cache';

This comment has been minimized.

@kamranayub

kamranayub Mar 2, 2018

Contributor

I wonder if this should also passthru cache-and-network.

This comment has been minimized.

@jbaxleyiii

jbaxleyiii Mar 23, 2018

Member

I don't think so since we want it to be forced through and cache-and-network may fire oddly

@Oudmane

This comment has been minimized.

Oudmane commented Mar 12, 2018

I'm facing the same issue here, When this can be PRed ?

@dennislaupman

This comment has been minimized.

dennislaupman commented Mar 12, 2018

Need this too!

@kamranayub

This comment has been minimized.

Contributor

kamranayub commented Mar 16, 2018

We've been using this in a patched client since I made these changes and everything's been stable. Hope to see this merged soon!

@razor-x

This comment has been minimized.

razor-x commented Mar 22, 2018

Is the benchmark test the only thing holding this up? What additional action is needed to merge?

@kamranayub

This comment has been minimized.

Contributor

kamranayub commented Mar 22, 2018

James Baxley
@@ -264,9 +265,13 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> {
}
// Override fetchPolicy for this call only
// only network-only and no-cache are safe to use
const isNetworkFetchPolicy =
fetchPolicy === 'network-only' || fetchPolicy === 'no-cache';

This comment has been minimized.

@jbaxleyiii

jbaxleyiii Mar 23, 2018

Member

I don't think so since we want it to be forced through and cache-and-network may fire oddly

@@ -1069,6 +1069,10 @@ export class QueryManager<TStore> {
reject(e);
return;
}
} else {
this.setQuery(queryId, () => ({
newData: { result: result.data, complete: true },

This comment has been minimized.

@jbaxleyiii

jbaxleyiii Mar 23, 2018

Member

I think this is a great solution honestly. Its a tricky one for sure with how react-apollo uses the cache directly so I think improvements can be made there as well. But great stuff!

@jbaxleyiii jbaxleyiii merged commit c4a1ad6 into apollographql:master Mar 23, 2018

13 of 14 checks passed

codecov/project 82.32% (target 83%)
Details
CLA Author has signed the Meteor CLA.
Details
Danger All green. Nice work.
Details
bundlesize Good job! bundle size < maxSize
Details
ci/circleci: Apollo Boost Your tests passed on CircleCI!
Details
ci/circleci: Apollo Cache Your tests passed on CircleCI!
Details
ci/circleci: Apollo Client Your tests passed on CircleCI!
Details
ci/circleci: Apollo InMemory Cache Your tests passed on CircleCI!
Details
ci/circleci: Apollo Utilities Your tests passed on CircleCI!
Details
ci/circleci: Danger Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Filesize Your tests passed on CircleCI!
Details
ci/circleci: GraphQL Anywhere Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details

@kamranayub kamranayub deleted the kamranayub:bug/GH-3030/no-cache-undefined branch Mar 23, 2018

@pencilcheck

This comment has been minimized.

pencilcheck commented Apr 9, 2018

I'm on 2.2.8 and data returned is not null but an empty object when I set to no-cache. Am I doing anything wrong?

The data returned successfully in any other policies.

@michaelknoch

This comment has been minimized.

michaelknoch commented Apr 20, 2018

i think this change hasn't been published yet @pencilcheck
Will there be a new release soon @jbaxleyiii ?

@OllieJennings

This comment has been minimized.

OllieJennings commented May 12, 2018

@jbaxleyiii I am still getting this issue on 2.3.1

@kristiehowboutdat

This comment has been minimized.

kristiehowboutdat commented May 15, 2018

Using fetchPolicy=no-cache for me also results in data: {}, even though the network response is present and visible.

Specifically, I end up with:

loading: false,
error: undefined,
data: {},
...

when using <Query fetchPolicy="no-cache" /> component .

   // package.json
    "apollo-cache-inmemory": "1.2.1",
    "apollo-client": "2.3.1",
    "apollo-codegen": "0.19.1",
    "apollo-link": "1.2.2",
    "apollo-link-error": "1.0.9",
    "apollo-link-http": "1.5.4",
    "react-apollo": "2.1.4"
@kristiehowboutdat

This comment has been minimized.

kristiehowboutdat commented May 16, 2018

Here is a reproduction, showing how the data is missing.
https://codesandbox.io/s/4zmopq25o0

It also demonstrates different results when defaultOptions versus setting the fetchPolicy component prop is used

@kamranayub

This comment has been minimized.

Contributor

kamranayub commented May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment