Skip to content
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

Use a type alias to allow bindings to take advantage of custom types #4412

Merged

Conversation

zzorba
Copy link
Contributor

@zzorba zzorba commented Dec 12, 2024

This PR adds a new type alias to the matrix-sdk-ffi crate called Date64. This is aliased to u64 for backwards compatibility (so no change to Swift/Kotlin as is), but as a distinct type it will allow uniffi integrations to customize the unpacking of the type automatically: https://mozilla.github.io/uniffi-rs/latest/udl/custom_types.html

Using this, it is possible to make the uniffi layer automatically convert these from u64 -> an appropriate system Date class in the generated code layer.

Today a u64 type can mean either a count or a date, so without an alias implementing a customType would not be possible. At the very least, the type helps improve the documentation, even if individual bindings choose not to adopt this in their UDL configs.

Signed-off-by: Daniel Salinas

@zzorba zzorba requested a review from a team as a code owner December 12, 2024 21:05
@zzorba zzorba requested review from andybalaam and removed request for a team December 12, 2024 21:05
@zzorba zzorba force-pushed the use_aliased_date64_for_uniffi_custom_types branch from 6d9533c to 4c7260c Compare December 12, 2024 21:09
@zzorba
Copy link
Contributor Author

zzorba commented Dec 12, 2024

I primarily want this for the react-native-matrix-sdk (Typescript), but I believe it is possible to add custom unpackers to Kotlin/Swift as well. That would of course be a breaking API change for any users of this library, the current iteration of this should fallback to u64 automatically.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.27%. Comparing base (54bd1d7) to head (69811df).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4412      +/-   ##
==========================================
- Coverage   85.27%   85.27%   -0.01%     
==========================================
  Files         282      282              
  Lines       31186    31186              
==========================================
- Hits        26594    26593       -1     
- Misses       4592     4593       +1     

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

@jplatte
Copy link
Collaborator

jplatte commented Dec 13, 2024

Small nitpick about the PR description, it talks about a marker trait but there is no new trait involved here, I think you meant "newtype"?

@andybalaam andybalaam requested review from poljar and removed request for andybalaam December 13, 2024 09:01
@Hywan Hywan requested review from Hywan and removed request for poljar December 13, 2024 09:07
Hywan
Hywan previously requested changes Dec 16, 2024
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Thanks, that's a nice improvement! I'm suggesting a renaming but everything else is nice!

bindings/matrix-sdk-ffi/src/utils.rs Outdated Show resolved Hide resolved
@zzorba zzorba force-pushed the use_aliased_date64_for_uniffi_custom_types branch from 4c7260c to 69811df Compare December 18, 2024 16:27
@zzorba zzorba requested a review from Hywan December 18, 2024 16:52
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Review comments were addressed and the PR seems fine to me as well.

@poljar poljar dismissed Hywan’s stale review December 20, 2024 09:46

Comment was addressed.

@poljar poljar merged commit f8a9d12 into matrix-org:main Dec 20, 2024
40 checks passed
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.

4 participants