[PM-24243] Add load_flags method to wasm PlatformClient #369
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #369 +/- ##
==========================================
+ Coverage 73.34% 73.93% +0.59%
==========================================
Files 252 252
Lines 21283 21581 +298
==========================================
+ Hits 15609 15955 +346
+ Misses 5674 5626 -48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| let js_value = serde_wasm_bindgen::to_value(&flags) | ||
| .expect("FlagsInput should always serialize successfully"); |
There was a problem hiding this comment.
We should try to avoid expect, as it would produce a panic. I don't think it would happen in this case but better safe than sorry.
| let js_value = serde_wasm_bindgen::to_value(&flags) | |
| .expect("FlagsInput should always serialize successfully"); | |
| let js_value = serde_wasm_bindgen::to_value(&flags).unwrap_or_default(); |
There was a problem hiding this comment.
Good to know! But this should no longer be needed after 9a7ca68
|
|
||
| #[derive(serde::Serialize, serde::Deserialize, Tsify)] | ||
| #[tsify(into_wasm_abi, from_wasm_abi)] | ||
| pub struct FlagsInput { |
There was a problem hiding this comment.
Rather than duplicate the Flags struct, we should tag the Flags struct in bitwarden_core with:
#[cfg_attr(
feature = "wasm",
derive(tsify::Tsify),
tsify(into_wasm_abi, from_wasm_abi)
)]
Otherwise we'll have to ensure we keep them both in sync, which is error prone.
There was a problem hiding this comment.
Agreed! Thanks @dani-garcia! Updated in 9a7ca68
I also had to make the Flags struct in bitwarden_core public and remove the #[cfg(feature="internal")] attribute in 8f2680b. If there's another or better way to go about it please let me know!
| #[allow(missing_docs)] | ||
| pub mod flags; |
There was a problem hiding this comment.
| #[allow(missing_docs)] | |
| pub mod flags; | |
| mod flags; |
You don't need to make this module pub, as you're already doing pub use flags::Flags a few lines after it.
Also both lines should be able to be tagged with the internal flag, the wasm crate is enabling it.
#[cfg(feature = "internal")]
mod flags;
#[cfg(feature = "internal")]
pub use flags::Flags;
There was a problem hiding this comment.
We intentionally didn't make flags public for mobile in order to avoid removal of flags from being a breaking change.
|
| #[allow(missing_docs)] | ||
| #[cfg(feature = "internal")] | ||
| pub fn set_flags(&self, flags: &Flags) { | ||
| *self.flags.write().expect("RwLock is not poisoned") = flags.clone(); | ||
| } |
There was a problem hiding this comment.
We already have load_flags above, and #[allow(missing_docs)] is not allowed on new functions.
🎟️ Tracking
PM-24243
📔 Objective
Add
load_flagsmethod to the wasmPlatformClientto match parity with the uniffiPlatformClientso that the TS clients can set the SDK flags dynamically.Related Clients PR: bitwarden/clients#15855
Added aFlagsInputstruct that mimics the internalFlagsstruct but with tsify bindings.Another potential option is to make the internalFlagsstruct public with wasm bindings to avoid duplicating the struct.⏰ Reminders before review
team
🦮 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 confirmedissue 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