-
Notifications
You must be signed in to change notification settings - Fork 0
Cache data fetches #14
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 93.84% 94.07% +0.22%
==========================================
Files 9 9
Lines 130 135 +5
Branches 39 41 +2
==========================================
+ Hits 122 127 +5
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
EmmaLRussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to use shallowRef - I don't think that existed when we used the freeze technique in HINT!
src/composables/useData.ts
Outdated
|
|
||
| const fetchErrors = ref<{ e: Error, message: string }[]>([]); | ||
| const histogramData = ref<DataRow[]>([]); | ||
| const histogramDataCache = shallowRef<Record<string, DataRow[]>>({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the cache needs to be a ref, shallow or otherwise.. The dict object reference itself isn't going to change and shouldn't need to be reactive..
But I think the histogramData is a perfect candidate to be a shallowRef since its top level reference will change every time the data selections change, but then the whole data structure should be fixed subsequently, so doesn't need to be deeply reactive/
Also, I really would be inclined to put both histogramData and histogramDataCache into the appStore, which is designed to be a shared data store. At the moment, if you were to call useData from multiple places in the code, you would end up with multiple copies of the data. I know that's not the plan but it still feels a bit worrisome... I think it's fine to still manage the fetching and caching from useData if you want to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now made a separate dataStore rather than put this all into the appStore, since I am worried about the appStore growing too big.
EmmaLRussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good.
If a data file has been requested already, this dict should prevent us from making the request again.
Using shallowRef to prevent Vue trying to watch the (very large) Object values.