perf(persist): subscriber calls persistQueryClientStore only on cache-related events#4884
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1e8c668:
|
There was a problem hiding this comment.
Thanks for putting this together 🚀
Is it worth reusing a predicate function here? Perhaps it's a hasty abstraction, but something along the lines of isCacheableEventType.
Edit: see suggestion here 🙂
| * Checks if emitted event is about cache change and not about observers. | ||
| * Useful for persist, where we only want to trigger save when cache is changed. | ||
| */ | ||
| function isCacheableEventType(eventType: NotifyEventType) { |
There was a problem hiding this comment.
@TkDodo I placed this function here as in internal one for now. Let me know if there is a better place for that.
There was a problem hiding this comment.
I think we should check that Dominik is happy with this idea, but I think we could start on the tests for the new behaviour if you're up for it 🙂
There was a problem hiding this comment.
sure I am. Although no idea where to start 😅
There was a problem hiding this comment.
It looks like the relevant tests are here:
I think this is probably going to be non-trivial to test. I think we'd essentially be looking to assert on when persistClient is and isn't called on the mocked persister, and probably on the state of the restored client. I think the interesting part here will be building and manipulating queries/mutations to get into the right state to make these assertions though.
I'd be happy to take a look but I won't have time for a little while, although I think Dominik said he'd be willing to lend a hand 🤞
Codecov ReportBase: 91.84% // Head: 91.87% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #4884 +/- ##
==========================================
+ Coverage 91.84% 91.87% +0.03%
==========================================
Files 111 111
Lines 4155 4171 +16
Branches 1079 1081 +2
==========================================
+ Hits 3816 3832 +16
Misses 318 318
Partials 21 21
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
TkDodo
left a comment
There was a problem hiding this comment.
the only thing missing for me is tests. What we would need is some sort of test that writes a custom persister that basically just tracks with jest.fn how often we call the persistence write function, then create an observer, mount and unmount, maybe set some options etc and then make sure that we call the write function not too often :)
| const CACHEABLE_EVENT_TYPES: Array<NotifyEventType> = [ | ||
| 'added', | ||
| 'removed', | ||
| 'updated', | ||
| ] |
There was a problem hiding this comment.
could we move this out of the function isCacheableEventType ? There's no need to re-create this every time.
| 'updated', | ||
| ] | ||
|
|
||
| return CACHEABLE_EVENT_TYPES.includes(eventType) |
There was a problem hiding this comment.
I'd also prefer if we'd name this just const cacheableEventTypes.
Thanks, it all makes sense. I'm not sure about my availability this week, but I hope I'll find some time to add it :) |
By default
persistQueryClientSubscribewas callingpersistQueryClientSaveon every single change in query cache.It was also triggered by events like
observerOptionsUpdatedwhich are emitted very often, usually on every single re-render of component/hook usinguseQuery.For performance improvement, we can limit list of events triggering
persistQueryClientSaveonly for those, which actually have influence on the cache itself.