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.

Sign up for GitHub

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

Jump to bottom

Consider cache.reset a destructive method #7150

Merged
merged 2 commits into from Oct 13, 2020
Merged

Consider cache.reset a destructive method #7150

merged 2 commits into from Oct 13, 2020

Conversation

joshjg
Copy link
Contributor

@joshjg joshjg commented Oct 11, 2020

Fixes #7138

See previous similar fixes for cache.evict (#6817) and cache.modify (#6898)

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

@joshjg
Copy link
Contributor Author

joshjg commented Oct 11, 2020

@benjamn - FYI, I wasn't able to write a failing test for this. I tried to model it off the test added in #6898 - but I couldn't even get your test to fail after removing wrapDestructiveCacheMethod.

@benjamn benjamn added this to the Post 3.0 milestone Oct 13, 2020
benjamn
benjamn approved these changes Oct 13, 2020
View reviewed changes
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.

LGTM—thanks @joshjg!

@benjamn benjamn merged commit 243d829 into apollographql:main Oct 13, 2020
benjamn added a commit that referenced this pull request Oct 13, 2020
@benjamn
Mention PRs #7150 and #7146 (both by @joshjg) in CHANGELOG.md.
cc9be37
@joshjg joshjg deleted the fix-7138 branch October 13, 2020 18:20
@benjamn benjamn mentioned this pull request Oct 13, 2020
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benjamn benjamn benjamn approved these changes

Assignees
No one assigned
Labels
💸 caching 🧞‍♂️ enhancement
Projects
None yet
Milestone
Post 3.0
Development

Successfully merging this pull request may close these issues.

resetStore causes queries with notifyOnNetworkStatusChange to return undefined
2 participants
@joshjg @benjamn