Skip to content

feat(ffi): Add ability to configure indexedb instead of sqlite for wasm platforms#5181

Closed
zzorba wants to merge 4 commits into
matrix-org:mainfrom
filament-dm:wasm_client_builder_session
Closed

feat(ffi): Add ability to configure indexedb instead of sqlite for wasm platforms#5181
zzorba wants to merge 4 commits into
matrix-org:mainfrom
filament-dm:wasm_client_builder_session

Conversation

@zzorba

@zzorba zzorba commented Jun 5, 2025

Copy link
Copy Markdown
Contributor

Introduces SqliteSessionBuilder and IndexedDbSessionBuilder to allow for a choice of session store. Non-wasm platforms will only support sqlite, while Wasm platforms will support indexedb. In order to maintain a compatible-ffi, these restrictions are handled at runtime rather than compile time.

We also conditionally remove things related to network features (proxy, ssl validation, etc), that are not available on wasm platforms.

  • Public API changes documented in changelogs (optional)

Signed-off-by: Daniel Salinas

@zzorba zzorba requested a review from a team as a code owner June 5, 2025 15:46
@zzorba zzorba requested review from Hywan and removed request for a team June 5, 2025 15:46
@zzorba zzorba changed the title Add ability to configure indexedb instead of sqlite for wasm platforms feat(wasm): Add ability to configure indexedb instead of sqlite for wasm platforms Jun 5, 2025
@zzorba zzorba force-pushed the wasm_client_builder_session branch from a1e186e to 3d39cdd Compare June 5, 2025 15:46
@jplatte

jplatte commented Jun 5, 2025

Copy link
Copy Markdown
Collaborator

PR title should probably make it clear that this is about the FFI crate :)

The challenge is to maintain a compatible -ffi API, despite the fact that
both cannot be used on both platforms.

Approach is to use runtime panics here to stop usage if it is being misused.
@zzorba zzorba force-pushed the wasm_client_builder_session branch from 3d39cdd to 9ace356 Compare June 5, 2025 15:57
@zzorba zzorba changed the title feat(wasm): Add ability to configure indexedb instead of sqlite for wasm platforms feat(wasm): Add ability to configure indexedb instead of sqlite for wasm platforms in ffi crate Jun 5, 2025
@codecov

codecov Bot commented Jun 5, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.86%. Comparing base (34d3cd4) to head (43911b8).
Report is 134 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5181      +/-   ##
==========================================
- Coverage   85.87%   85.86%   -0.02%     
==========================================
  Files         338      338              
  Lines       37070    37070              
==========================================
- Hits        31834    31830       -4     
- Misses       5236     5240       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Hywan Hywan self-assigned this Jun 5, 2025
@Hywan Hywan changed the title feat(wasm): Add ability to configure indexedb instead of sqlite for wasm platforms in ffi crate feat(ffi): Add ability to configure indexedb instead of sqlite for wasm platforms Jun 6, 2025

@Hywan Hywan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch.

First off, I believe you should split all the modifications from the Cargo.toml file into its own patch.

Second, I think you can simplify things by introducing a type alias SessionConfig. We don't want to expose SQLite or IndexedDb directly, we want to hide that from the consumer/user. See my suggestions.

What do you think?


[lib]
crate-type = ["cdylib", "staticlib"]
crate-type = ["cdylib", "staticlib", "lib"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's related to what the patch says it does. Can you split this in another patch please?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this seems weird, what's the idea with this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uniffi bindings generator requires the library to be built as a "lib" so it can be included in a second generated-rust library that is fed through. But I will break that out into a second PR.

Comment on lines +82 to +93
[target.'cfg(target_family = "wasm")'.dependencies.matrix-sdk]
workspace = true
features = [
"anyhow",
"e2e-encryption",
"experimental-widgets",
"markdown",
"rustls-tls",
"socks",
"indexeddb",
"uniffi",
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes in Cargo.toml just be in a dedicated patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened up #5211 which is nearly merged. I will rebase this branch with those changes (and other feedback) once it lands.

Comment thread bindings/matrix-sdk-ffi/src/client_builder.rs Outdated
Comment thread bindings/matrix-sdk-ffi/src/client_builder.rs
pub fn username(self: Arc<Self>, username: String) -> Arc<Self> {
#[derive(Clone, uniffi::Object)]
pub struct IndexedDbSessionBuilder {
#[cfg_attr(not(target_family = "wasm"), allow(unused))]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use IndexedDb outside Wasm, all fields can be present.

Comment on lines +399 to +408
#[derive(Clone)]
pub enum ClientSessionConfig {
/// Setup the client to use the SQLite store.
#[cfg_attr(target_family = "wasm", allow(unused))]
Sqlite(SqliteSessionBuilder),

/// Setup the client to use the IndexedDB store.
#[cfg_attr(not(target_family = "wasm"), allow(unused))]
IndexedDb(IndexedDbSessionBuilder),
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove all this, and add:

Suggested change
#[derive(Clone)]
pub enum ClientSessionConfig {
/// Setup the client to use the SQLite store.
#[cfg_attr(target_family = "wasm", allow(unused))]
Sqlite(SqliteSessionBuilder),
/// Setup the client to use the IndexedDB store.
#[cfg_attr(not(target_family = "wasm"), allow(unused))]
IndexedDb(IndexedDbSessionBuilder),
}
#[cfg(target_family = "wasm")]
pub type SessionConfig = IndexedDbSessionBuilder;
#[cfg(not(target_family = "wasm"))]
pub type SessionConfig = SqliteSessionBuilder;


#[derive(Clone, uniffi::Object)]
pub struct ClientBuilder {
session: Option<ClientSessionConfig>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then…

Suggested change
session: Option<ClientSessionConfig>,
session: Option<SessionConfig>

Comment on lines 468 to 478
pub fn session_sqlite(self: Arc<Self>, config: Arc<SqliteSessionBuilder>) -> Arc<Self> {
let mut builder = unwrap_or_clone_arc(self);
builder.homeserver_cfg = Some(HomeserverConfig::ServerName(server_name));
builder.session = Some(ClientSessionConfig::Sqlite(config.as_ref().clone()));
Arc::new(builder)
}

pub fn homeserver_url(self: Arc<Self>, url: String) -> Arc<Self> {
pub fn session_indexeddb(self: Arc<Self>, config: Arc<IndexedDbSessionBuilder>) -> Arc<Self> {
let mut builder = unwrap_or_clone_arc(self);
builder.homeserver_cfg = Some(HomeserverConfig::Url(url));
builder.session = Some(ClientSessionConfig::IndexedDb(config.as_ref().clone()));
Arc::new(builder)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And finally…

Suggested change
pub fn session_sqlite(self: Arc<Self>, config: Arc<SqliteSessionBuilder>) -> Arc<Self> {
let mut builder = unwrap_or_clone_arc(self);
builder.homeserver_cfg = Some(HomeserverConfig::ServerName(server_name));
builder.session = Some(ClientSessionConfig::Sqlite(config.as_ref().clone()));
Arc::new(builder)
}
pub fn homeserver_url(self: Arc<Self>, url: String) -> Arc<Self> {
pub fn session_indexeddb(self: Arc<Self>, config: Arc<IndexedDbSessionBuilder>) -> Arc<Self> {
let mut builder = unwrap_or_clone_arc(self);
builder.homeserver_cfg = Some(HomeserverConfig::Url(url));
builder.session = Some(ClientSessionConfig::IndexedDb(config.as_ref().clone()));
Arc::new(builder)
}
pub fn session(self: Arc<Self>, config: Arc<SessionConfig>) -> Arc<Self> {
let mut builder = unwrap_or_clone_arc(self);
builder.session = Some(config);
Arc::new(builder)
}

zzorba and others added 3 commits June 9, 2025 09:04
Co-authored-by: Ivan Enderlin <ivan@mnt.io>
Signed-off-by: Daniel Salinas <zzorba@users.noreply.github.com>
Conditionalize uniffi exports when this flag is not present
@zzorba

zzorba commented Jun 18, 2025

Copy link
Copy Markdown
Contributor Author

Going to close this in favor of #5211 and #5245

@zzorba zzorba closed this Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants