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

Rust SDK: prefer using interfaces when they exist #1829

Closed
wants to merge 3 commits into from

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Nov 16, 2023

Draft, WIP.

The idea is to use the interfaces declared in the FFI layer as much as possible.

Sometimes the object has to be cast to be able to use FFI API. Also the FFI layer does not return systematically interface but sometimes the implementation type.

Adding useAny since the interface does not extend Disposable. Ideally it could be the case, but not a blocker, since we have the handy Disposable.destroy(...) to help (could be renamed to use but I prefer to have a different name for clarity).

I think I have use all the possible interface and at all places, but I may have missed some.

This work will help to unit test our implementation, but allowing to inject our own test implementation of the interfaces.

Copy link
Contributor

github-actions bot commented Nov 16, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/zidDpZ

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f80ec8a) 63.40% compared to head (234dd19) 63.40%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1829   +/-   ##
========================================
  Coverage    63.40%   63.40%           
========================================
  Files         1293     1293           
  Lines        33628    33628           
  Branches      6982     6982           
========================================
  Hits         21322    21322           
  Misses        9117     9117           
  Partials      3189     3189           

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

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jmartinesp
Copy link
Member

jmartinesp commented Nov 17, 2023

This work will help to unit test our implementation, but allowing to inject our own test implementation of the interfaces.

Take into account that some of the Rust binding interfaces return or use concrete FFI class implementations as parameters, not the interfaces, so even if we could create fakes of them we'd still need to create fakes of those objects. The good news is, a couple of weeks ago a PR was merged in uniffi about making the classes open so we can fake those too as long as we don't pass them to actual Rust objects. Bad news is, I think we haven't updated to a version with that change in the Rust SDK yet.

@bmarty
Copy link
Member Author

bmarty commented Nov 17, 2023

Take into account that some of the Rust binding interfaces return or use concrete FFI class implementations as parameters, not the interfaces

Yes you're right, my plan is to list all those cases (some of them are TODOs in my PR) and share it with the Rust team.

For the parameter, I think the API could work with ids. For instance if the parameter is of type Room, it could be changed to RoomInterface, but it's probably better to just pass the roomId: String.

@jmartinesp
Copy link
Member

Honestly, before changing the API I'd just wait for the result of the PR I mentioned above to reach the Rust SDK so we don't end up changing any APIs we don't really need to. Fetching stuff from the Rust SDK with ids may not be free of side-effects, since we also need to take into account their lifetime in our code and if the backing Rust object should be kept alive, or if the object auto-starts some logic by instantiating it.

@bmarty
Copy link
Member Author

bmarty commented Nov 17, 2023

the PR I mentioned above

Sorry I do not see this PR, which one is it?

@jmartinesp
Copy link
Member

It's this one. I submitted it some time ago, along with another version that used interfaces instead and the uniffi team preferred this version with open classes, as it was easier to implement and maintain.

So with this we could just create fakes by extending the classes, and we wouldn't need to change to using identifiers and fetching new instances.

@bmarty
Copy link
Member Author

bmarty commented Dec 13, 2023

Will revisit this using the open modifier added to all the Uniffi classes.

@bmarty bmarty closed this Dec 13, 2023
@bmarty bmarty deleted the feature/bma/useRustInterfaces branch October 21, 2024 12:40
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.

2 participants