-
Notifications
You must be signed in to change notification settings - Fork 196
fix: avoid fetching the user's addressbooks per component instance #4794
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
base: main
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| this.promise = (async () => { | ||
| await client.connect({ enableCardDAV: true }) |
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.
This isn’t exactly what Richard suggested, but I dropped that approach because I couldn’t get it to work. In hindsight, I think the idea itself was fine, but I realized too late that the Vuex store was already shared between components, while the Pinia instances were not. Since I created a new Pinia store for it, that was probably the issue I was running into.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4794 +/- ##
=========================================
Coverage 11.04% 11.04%
Complexity 293 293
=========================================
Files 127 127
Lines 6565 6563 -2
Branches 1204 1204
=========================================
Hits 725 725
+ Misses 5718 5716 -2
Partials 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/store/addressbooks.js
Outdated
| return context.getters.getAddressbooks | ||
| } | ||
|
|
||
| context.state.addressbooksFetched = true |
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.
The shared pinia store + this should do the trick, right? The promise hack seems superfluous
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'm afraid not. The shared pinia store is the requirement to make the promise hack work 🙈
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'm going to revert this line as it's actually unrelated.
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.
Huh. I was pretty sure that moving the flag toggle up could solve the issue elegantly. The first call of the action actually fetches addressbooks the other ones return early. This is fine if the result of the action is not used but address books populate reactively once loaded.
Signed-off-by: Daniel Kesselberg <[email protected]>
99ee72a to
7bbe969
Compare
How to test:
Before: You should see one request for each recipient to
PROPFIND /remote.php/dav/principals/users/alice/andPROPFIND /remote.php/dav/addressbooks/users/alice/After: You should see only one request to
PROPFIND /remote.php/dav/principals/users/alice/andPROPFIND /remote.php/dav/addressbooks/users/alice/