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

Fix QueryManager for queries with fetchPolicy="no-cache" #4573

Conversation

bradleyayers
Copy link
Contributor

@bradleyayers bradleyayers commented Mar 13, 2019

This PR fixes a bug with fetchPolicy="no-cache", where data initially appears, and then subsequently disappears (e.g. #3452 (comment)). I was able to reproduce the issue:

  1. Use InMemoryCache.

  2. Make a query with fetchPolicy="no-cache". It will resolve correctly and the data will be available.

  3. While that query is still being observed (via ObservableQuery), make a separate query using cache-and-network (I don't think the fetchPolicy is actually significant here).

  4. When the new query updates the cache:

    1. QueryManager#broadcastQueries() is called.
    2. A cache watch's callback (which was incorrectly setup when the no-cache query was created) is executed and calls this.setQuery(queryId, () => ({ invalidated: true, newData }));
    3. Because the query isn't in the cache, newData is empty, but the query is updated anyway.
    4. The existing ObservableQuery for the no-cache query has the empty result emitted.

I'm not familiar enough with this codebase to know how to write a test for this, so it'd be great if someone else could jump in and help with that.

Relates to #3452.

Questions:

  1. Why does this.dataStore.getCache().watch({…}) trigger for the no-cache query when a different (unrelated) query updates the cache?

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

@davewthompson
Copy link

Can this be fixed? We're seeing issues with it.

@bradleyayers
Copy link
Contributor Author

@benjamn is there anything else I should be doing with this to move it forward?

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @bradleyayers; sorry for the delay!

@benjamn benjamn force-pushed the fix/no-cache-clobber-after-rebroadcastt branch from 0a3bb40 to 6347887 Compare April 8, 2019 17:34
@benjamn benjamn self-assigned this Apr 8, 2019
@benjamn benjamn merged commit 58cb6d5 into apollographql:master Apr 8, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 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.

3 participants