-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-11764] Implement account switching and sdk initialization #11472
[PM-11764] Implement account switching and sdk initialization #11472
Conversation
…Client$` function
New Issues
Fixed Issues
|
Reverting to draft while I add some logic to clean up the cache |
tap({ | ||
finalize: () => { | ||
if (this.sdkClientCache.has(userId)) { | ||
this.sdkClientCache.delete(userId); | ||
} | ||
}, | ||
}), |
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.
thought: I'm not 100% happy with this, but I can't really see any better solutions short-of removing the cache all together or maybe writing some advanced separate utility for it. Caching is hard.
An alternative would be to remove this and let the cache grow (leak memory). I don't think it would ever be an issue since nobody logs in and out of so many accounts that it will actually become an issue, but @JaredSnider-Bitwarden challenged me on this approach and in the end I leaned towards not leaking.
Builds breaking because the PR still depends on the old SDK version. We'll have to merge the SDK PR first |
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.
Auth changes LGTM! Thank you for the tweaks based on our discussion.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11472 +/- ##
==========================================
- Coverage 33.19% 33.19% -0.01%
==========================================
Files 2772 2779 +7
Lines 86137 86362 +225
Branches 16411 16452 +41
==========================================
+ Hits 28594 28666 +72
- Misses 55278 55419 +141
- Partials 2265 2277 +12 ☔ View full report in Codecov by Sentry. |
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.
Looks really good, just a couple minor suggestions and comments.
You have a todo in here for figure out what happens on logout but I think you've already covered it. A null user key should be saved for the user and any cached sdk clients should be freed.
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.
Looks great!
🎟️ Tracking
Depends on: bitwarden/sdk-sm#1116
📔 Objective
Adds a function to default SDK service which can be used to subscribe to a fully initialized.
Note: Keys seem to hang around in memory, most likely because of how garbage collection works. Explicit clean up will be implemented in a follow up PR PM-13260 Implement cleanup logic for SDK on account logout.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes