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

Eliminate cache.writeData. #5923

Merged
merged 3 commits into from
Mar 2, 2020
Merged

Eliminate cache.writeData. #5923

merged 3 commits into from
Mar 2, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 7, 2020

In PR #5909, I referred to cache.writeData as a "foot-seeking missile" because it's one of the easiest ways to turn your faulty assumptions about how the cache represents data internally into cache inconsistency and corruption.

PR #5909 introduced an alternative api, cache.modify(id, modifiers), which aims to take the place of the more "surgical" uses of cache.writeData. Somewhat to my surprise, in almost every case where cache.writeData was used in our tests, an appropriate query was already sitting very close by, making cache.writeQuery just as easy to call.

If you think your life is worse now that you have to pass a query to cache.writeQuery or a fragment to cache.writeFragment, please realize that cache.writeData was dynamically creating a fresh query or fragment behind the scenes, every time it was called, so it was actually doing a lot more work than the equivalent call to cache.writeQuery or cache.writeFragment.

Left to do:

  • Update the relevant documentation to stop recommending cache.writeData.
  • Put a note in CHANGELOG.md.
  • Show an example of moving from cache.writeData to cache.writeQuery in the Migration Guide.

@benjamn benjamn added this to the Release 3.0 milestone Feb 7, 2020
@benjamn benjamn self-assigned this Feb 7, 2020
@benjamn benjamn changed the title Eliminate cache.writeData. :feelsgood: Eliminate cache.writeData. Feb 7, 2020
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

🎉 @benjamn! The code changes look awesome as usual. I added one small Migration Guide mention in the TODO list.

@benjamn benjamn changed the base branch from EntityStore-modify-method to master February 10, 2020 16:00
@bwhitty
Copy link
Contributor

bwhitty commented Feb 17, 2020

Huh, stumbling upon this is exciting. We're doing a deep integration into client-side cache management and local state/resolvers and it was never truly clear to me why all the different cache modification APIs existed.

I referred to cache.writeData as a "foot-seeking missile" in PR #5909,
because it's one of the easiest ways to turn your faulty assumptions about
how the cache represents data internally into cache corruption.

PR #5909 introduced an alternative api, cache.modify(id, modifiers), which
aims to take the place of the more "surgical" uses of cache.writeData.
However, as you can see, in almost every case where cache.writeData was
used in our tests, an appropriate query was already sitting very close by,
making cache.writeQuery just as easy to call.

If you think your life is worse now that you have to pass a query to
cache.writeQuery or a fragment to cache.writeFragment, please realize that
cache.writeData was dynamically creating a fresh query or fragment behind
the scenes, every time it was called, so it was actually doing a lot more
work than the equivalent call to cache.writeQuery or cache.writeFragment.
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #5923 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5923      +/-   ##
==========================================
- Coverage   95.24%   95.20%   -0.04%     
==========================================
  Files          89       88       -1     
  Lines        3703     3674      -29     
  Branches      909      903       -6     
==========================================
- Hits         3527     3498      -29     
  Misses        154      154              
  Partials       22       22              

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 0db0b07...36559aa. Read the comment docs.

@hwillson
Copy link
Member

hwillson commented Mar 2, 2020

We should be all set here @benjamn. I have several more local state section changes coming, but since they aren't specifically related to writeData, I'll get them into separate PR's. Thanks!

@hwillson hwillson merged commit f26b98c into master Mar 2, 2020
@hwillson hwillson deleted the eliminate-cache.writeData branch March 2, 2020 16:00
@stubailo
Copy link
Contributor

stubailo commented Mar 6, 2020

Neat!

@duskmoon314
Copy link

I was following the doc about local state management which still includes client.writeData. I think this is a small mistake and needs to be solved.

@drewkht
Copy link

drewkht commented Jul 2, 2020

@duskmoon314 They have updated documentation for these changes in the v3.0 beta branch.

@AndreiRailean
Copy link

this document still talks about cache.writeData https://www.apollographql.com/docs/tutorial/local-state

@hwillson
Copy link
Member

Thanks @AndreiRailean - we're working on changes to the tutorial to address this.

@nisarhassan12
Copy link

Thanks, @hwillson this is my first time with the apollo-client was following the tutorial https://www.apollographql.com/docs/tutorial/local-state to learn it which uses cache.writeData.

I was not able to figure out why it's not working.

I'm looking forward to the update in the tutorial. Thanks.

@idkjs
Copy link

idkjs commented Sep 15, 2020

Are there docs on the cache.modify() change anywhere? Thank you.

@idkjs
Copy link

idkjs commented Sep 15, 2020

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants