Add "Converting an existing Component to use UniFFI" howto [ci skip] #4004
Conversation
| ``` | ||
|
|
||
| In future, we intend to automatically extract documentation from the Rust code | ||
| and make it easily available to consumers of the generated bindings. |
There was a problem hiding this comment.
What I would like to write here is "run this cool script to copy the doc comments over into your UDL file" but I'm not sure that can be run by anyone but me currently. I'll take a look at what would be required to let that be runnable by someone else...
There was a problem hiding this comment.
@mhammond what do you think, is it worth me trying to make the automagical "copy docstrings from the Rust code into the UDL" functionality from mozilla/uniffi-rs#416 into something more generally usable, or is that just getting ahead of ourselves?
There was a problem hiding this comment.
In one day? :) I think would be valuable, but not at all urgent. Maybe leaving a breadcrumb here would be worthwhile?
| As we've gained more experience with building components in this way, we've | ||
| started to automated bindings generation and capture best practices in a | ||
| tool called [UniFFI](https://mozilla.github.io/uniffi-rs/), which is the | ||
| current recommended approach when [adding a new component from scratch]( |
There was a problem hiding this comment.
trivial nit that's probably not even correct - 'currently'?
| * The low-level `LibComponentNameFFI.kt` file will disappear entirely, as will some of the | ||
| code that converts it back into nice high-level Kotlin classes and interfaces. | ||
| * Some of the hand-written Kotlin code may remain, if it provides functionality that | ||
| cannot be implemented in Rust. |
There was a problem hiding this comment.
nit: indentation (I only mention this because I'm too lazy to check whether it is rendered OK or not - ignore if so)
There was a problem hiding this comment.
it did not render ok, thanks for noticing!
| The goal here is to replace much of the hand-written wrapper layers with autogenerated | ||
| code: | ||
|
|
||
| * The `./ffi/` crate will disappear entirely, its work is automated by UniFFI |
There was a problem hiding this comment.
Unless I misunderstand, if you are only doing a partial conversion, this might remain, right? If that's true, I think it might be worth setting the scene for that above - there are lots of places below which assume "will be removed", so I don't think it's worth trying to qualify that everwhere, but calling it out as an intro to this section might make sense.
There was a problem hiding this comment.
That's a good point, I hadn't really considered partial conversions here...I guess that would be for the case if there's something that UniFFI doesn't support yet, and you need to write your own pub extern "C" functions?
I think I would still argue that those functions should live in the main crate and be part of lib.rs and your top-level public API, rather than squirreled away in a separate ./ffi/ crate. But I could add some words to this effect if you think it would be helpful?
(In fact I don't really recall why we settled on this convention of having a separate ffi crate at all to be honest...)
There was a problem hiding this comment.
Yes, that would be perfect - just a small note saying "things which are unable to be converted should be moved into lib.rs" would be perfect and about all the detail needed here.
| cannot be implemented in Rust. | ||
| * The low-level `RustComponentNameAPI.h` file will disappear entirely, as will some of the | ||
| code that converts it back into nice high-level Swift classes and interfaces. | ||
| * Some of the hand-written Kotlin code may remain, if it provides functionality that |
There was a problem hiding this comment.
ditto indentation and s/Kotlin/Swift/ here?
| it was using Protocol Buffers to serialize data to pass over the FFI. The message types | ||
| defined in the `.proto` file will need to be converted into `dictionary` or `enum` definitions | ||
| in your `.udl` file. See the section below for more details. | ||
|
|
There was a problem hiding this comment.
I'd consider adding a new point along the lines of "Don't be too concerned about making small changes to the 'spelling' of the API - if there are small differences in the names generated by uniffi vs what is in the hand-written FFI, it's probably in everyones interest to take that hit and update the consumers." - I see you do make that point a bit later, but IMO it's worth introducing it here.
There was a problem hiding this comment.
I'm going to add this under the earlier "write a draft of the .udl" section and then reinforce it again at several points further down.
| If it happen to find that you've deleted the last crate from the list in `protobuf_files.toml`, | ||
| congratulations! You've successfully removed protocol buffers from this repo entirely, and should | ||
| file a bug to track the complete removal of protobuf from our tooling and dependency chain. | ||
|
|
There was a problem hiding this comment.
Ditto the "Don't be concerned about small naming changes here" or similar?
| previous hand-written ones. For example, UniFFI insists on CAMEL_CASING variant names in | ||
| Kotlin enums while the hand-written code does not to this consistently. Some components | ||
| also have small naming differences between the Rust code and the hand-written Kotlin bindings, | ||
| which UniFFI will not allow. |
There was a problem hiding this comment.
If you take the previous "don't be afraid" suggestions, maybe add here "As noted above"?
|
I'm going to go ahead and merge this (with nits addressed) so that I don't leave it hanging around while I'm on PTO, but @lougeniaC64 any feedback you may have is still very welcome. |
Rendered View
@lougeniaC64 @mhammond, pinging you both for review here because I hope you will both find yourselves either doing this or walking someone else through it in the near-to-mid-term future.