feat(wasm): add feature support for indexedb and sqlite to matrix-sdk-ffi#5245
feat(wasm): add feature support for indexedb and sqlite to matrix-sdk-ffi#5245zzorba wants to merge 18 commits intomatrix-org:mainfrom
Conversation
|
Waiting on #5211 to be merged before reviewing this one. |
|
Would be nice to get opinions on the new session_store and client_builder changes. Those are wholly new, and structural feedback would be very welcome |
|
Can this one be rebased as well? |
eacaec7 to
f431d9a
Compare
|
Yep, rebased it to adopt the Cargo.toml changes. This PR now concerns only the configuration of the session store. Let me know if a different approach would be preferred here, adding sub-builders seemed more self-documenting than having a large number of 'optional' methods on the main builder, but happy to do it a different way if you prefer. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5245 +/- ##
=======================================
Coverage 90.18% 90.18%
=======================================
Files 334 334
Lines 104822 104822
Branches 104822 104822
=======================================
+ Hits 94534 94538 +4
+ Misses 6236 6232 -4
Partials 4052 4052 ☔ View full report in Codecov by Sentry. |
|
Here is a corresponding complement-crypto PR: matrix-org/complement-crypto#195 |
7ac8f7c to
474eb29
Compare
The system of platform targets was already quite messy, and becoming even worse as we start preparing for Wasm support. Switch to features instead to make this easier to work with.
474eb29 to
fe98e31
Compare
| --exclude matrix-sdk-crypto --exclude xtask | ||
| --no-default-features | ||
| --features native-tls,sso-login,testing | ||
| --features native-tls,sso-login,sqlite,testing |
There was a problem hiding this comment.
The same patch is mixing two things. Proof is your commit message contains “and”.
Please split this patch into two.
There was a problem hiding this comment.
By the way, this change should be a fixup of the patch introducing the sqlite feature.
There was a problem hiding this comment.
It is not mixing two different things, it is adding support for indexedb which requires parameterizing the sqlite configuration. I do not believe these are separable changes.
| #[uniffi::constructor] | ||
| pub fn new(data_path: String, cache_path: String) -> Self { | ||
| Self { | ||
| pub fn new(data_path: String, cache_path: String) -> Arc<Self> { |
There was a problem hiding this comment.
I don't know what that means. What is a fixup patch?
There was a problem hiding this comment.
A fixup patch is a patch that is “melt”/“merged” with another one so that we end up with a single patch, see https://git-scm.com/docs/git-rebase.
There was a problem hiding this comment.
The contributing guide also explains it: https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#addressing-review-comments-using-fixup-commits.
| use std::sync::Arc; | ||
|
|
||
| use super::SessionStoreResult; | ||
| use crate::{client_builder::ClientBuildError, helpers::unwrap_or_clone_arc}; | ||
|
|
Co-authored-by: Ivan Enderlin <ivan@mnt.io> Signed-off-by: Daniel Salinas <zzorba@users.noreply.github.com>
Co-authored-by: Ivan Enderlin <ivan@mnt.io> Signed-off-by: Daniel Salinas <zzorba@users.noreply.github.com>
Co-authored-by: Ivan Enderlin <ivan@mnt.io> Signed-off-by: Daniel Salinas <zzorba@users.noreply.github.com>
Co-authored-by: Ivan Enderlin <ivan@mnt.io> Signed-off-by: Daniel Salinas <zzorba@users.noreply.github.com>
|
I will address the feedback myself and will merge the PR. |
|
Closing in favor of #5811. |
Expose a choice of session storage in the matrix-sdk-ffi crate. The initial choices are Sqlite (supported on non-Wasm platforms) and IndexedDb (Wasm only). It was agreed though that feature flags make more sense here, and make the code more maintainable.
The existing bundled-sqlite is setup to opt-in sqlite, however several methods for configuring a Sqlite store have been moved onto a sub-builder for organizational reasons.
This PR is stacked on top of https://github.com/matrix-org/matrix-rust-sdk/pull/5211/files, I will rebase it against main when that has landed.
Corresponding PR for complement-crypto: matrix-org/complement-crypto#195
Signed-off-by: Daniel Salinas