-
Notifications
You must be signed in to change notification settings - Fork 357
IndexedDB: upgrade indexed_db_futures dependency
#5722
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
Conversation
Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
…nd errors for each store Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
…firefox Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5722 +/- ##
=======================================
Coverage 88.40% 88.41%
=======================================
Files 359 359
Lines 99060 99060
Branches 99060 99060
=======================================
+ Hits 87574 87579 +5
+ Misses 7347 7342 -5
Partials 4139 4139 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #5722 will not alter performanceComparing Summary
|
Signed-off-by: Michael Goldenberg <m@mgoldenberg.net>
Hywan
left a comment
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.
Please, use the Matrix Rust SDK's fork. Apart from that, I think we are good! Thanks the tremendous work!
Hywan
left a comment
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 good to me.
| .get_all() | ||
| .await? | ||
| .iter() | ||
| .filter_map(Result::ok) |
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.
It means that we ignore rows containing an error. I'm okay with that, but I don't know if that was the behaviour before though.
| .get_all() | ||
| .await? | ||
| .iter() | ||
| .filter_map(Result::ok) |
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.
Same here.
NOTE: this should not be merged until matrix-org/rust-indexed-db#1 is merged! The
[patch]in this branch should point to the officialmatrix-orgfork ofrust-indexed_db, but is currently pointed at my personal fork.Background
This pull request makes updates
indexed_db_futuresin thematrix-sdk-indexeddbcrate. The reason we'd like to update this dependency is because the version currently used does not fully support the Chrome browser (see #5420).The latest version of
indexed_db_futureshas significant changes. Many of these changes can be integrated without issue. There is, however, a single change which is incompatible with thematrix-sdk-indexeddbcrate. Namely, one cannot access the active transaction in the callback to update the database (for details, see Alorel/rust-indexed-db#66).An Updated Proposal
Originally, new migrations were implemented in order to work around this issue (see #5467). However, the proposal was ultimately rejected (see @andybalaam's comment).
For this reason, the dependency has instead been
[patch]ed in the top-levelCargo.tomlwith a modified version ofindexed_db_futures(see matrix-org/rust-indexed-db#1). Furthermore, these changes have been proposed to the maintainer and are awaiting feedback (see Alorel/rust-indexed-db#72).Why do we need the active transaction in our migrations?
The
crypto_storemodule provides access to the active transaction to its migrations (see here). Furthermore, there is a single migration (v11_to_v12) in thecrypto_storemodule which actually makes use of the active transaction (see here).For clarity, the reason
v11_to_v12is problematic in the latest versions ofindexed_db_futuresis because it is simply adding an index to an object store which was created in a different migration and this requires access to the active transaction. All the other migrations create object stores and indices in the same migration, which does not suffer from the same issue.Changes
indexed_db_futuresto the workspaceCargo.tomland add a[patch]so that it points to a modified version.GenericErrortype and conversions in order to more easily mapindexed_db_futureserrors intomatrix-sdk-*errors.indexed_db_futureswasm-packtests against ChromeCloses #5420.
Signed-off-by: Michael Goldenberg m@mgoldenberg.net