Skip to content

Conversation

oguzkocer
Copy link
Contributor

This PR builds on #35 and should be merged after it, so it's marked as Draft, but it's otherwise ready to be merged.

In #28, we'd like to generate different objects depending on different contexts. In UDL based design, even if we generate the Rust code, we'd also have to generate the UDL code alongside it. That's not something readily available, so instead, we are going to use the uniffi procmacros. This will allow us to generate only the Rust code and then derive the uniffi bindings from it.

There are additional benefits to using macros. Most importantly, debugging UDL errors can be time consuming because it's not parsed at a proper language level, so even a missing ; can result in hard to identify errors. It also makes it so that we only have 1 source of truth, and that's the Rust code. Finally, it simplifies the process of adding new types, because there is only one place to add them.

Of course this decision does have its own drawbacks, mainly UDL being a more accessible format for developers - as opposed to the Rust code. We thought this was an acceptable compromise because we'll have documentation for the API, so developers won't need to consult the source code. We also have native bindings and will introduce native wrappers that work on a higher level.


The conversion was fairly simple and all I had to do was adjust a few things. One minor drawback I found was that uniffi doesn't currently have None (or null) as a possible default value. That means using null as a way to signal "use the default value set by the server" doesn't work - not unless we are willing to pass in a bunch of null values from our native wrappers. That resulted in the following change:

// before
pub struct PostListParams {
    pub page: Option<u32>,
    pub per_page: Option<u32>,
}

// after
pub struct PostListParams {
    #[uniffi(default = 1)]
    pub page: u32,
    #[uniffi(default = 10)]
    pub per_page: u32,
}

This sets the default values for these parameters to the default values set by the API. I don't think this change is good or bad, as it's just a different way of handling the default. So, I didn't see this as something concerning - and even if I did - I think we can just contribute to uniffi to add the possibility of using None as a default value. For now, I think this is a good implementation and we should proceed with it.

@oguzkocer oguzkocer added this to the 0.1 milestone Mar 7, 2024
@oguzkocer oguzkocer requested a review from jkmassel March 7, 2024 19:00
@crazytonyli
Copy link
Contributor

That means using null as a way to signal "use the default value set by the server" doesn't work - not unless we are willing to pass in a bunch of null values from our native wrappers.

I don't see passing null values is a big issue, considering we'll need to do that on the response objects, i.e. PostObject. The request objects would have way less fields.

But it's be nice to generate initialisers to use nil as default value. Not sure if that's a supported feature:

// wp_api.swift
public struct PostListParams {

    public init(
        page: UInt32? = nil, 
        perPage: UInt32? = nil) {
        self.page = page
        self.perPage = perPage
    }
}

@oguzkocer
Copy link
Contributor Author

But it's be nice to generate initialisers to use nil as default value. Not sure if that's a supported feature:

That's exactly what I was referring to in the PR description:

One minor drawback I found was that uniffi doesn't currently have None (or null) as a possible default value.

This was possible with UDL, but not yet supported in procmacros.

@oguzkocer
Copy link
Contributor Author

But it's be nice to generate initialisers to use nil as default value. Not sure if that's a supported feature:

I was thinking a bit more about this, and I think it might be possible to handle it in a different way. Rust has a Default trait for these types of things and I think we can expose its implementation through the FFI.

If I am not mistaken, that'd mean that we'll be able to call PostListParams.default() - but I'd have to check.

In any case, since this is not the main point for this PR, I think we should keep the implementation as is for now. At some point, I'll make a proposal about this topic with all the different options I am aware of. How does that sound @crazytonyli?

Base automatically changed from add/ci to trunk March 11, 2024 16:52
@jkmassel jkmassel force-pushed the switch-to-uniffi-procmacro branch from 555bd2c to c7968fb Compare March 11, 2024 17:00
@jkmassel
Copy link
Contributor

Rust has a Default trait for these types of things and I think we can expose its implementation through the FFI.

Either way, I'd imagine we'll want to implement them for many of these types to make instantiation more simple in the default case?

since this is not the main point for this PR, I think we should keep the implementation as is for now

I agree about handling this in a separate PR

@jkmassel jkmassel marked this pull request as ready for review March 11, 2024 17:06
@oguzkocer oguzkocer merged commit 312c4c5 into trunk Mar 11, 2024
@oguzkocer oguzkocer deleted the switch-to-uniffi-procmacro branch March 11, 2024 18:20
@crazytonyli
Copy link
Contributor

That's exactly what I was referring to in the PR description:

One minor drawback I found was that uniffi doesn't currently have None (or null) as a possible default value.

Oh nice! I didn't check out the generated Swift code. I thought the [default=xxx] directive would assign a default value at the property declaration statement. Something like:

public struct PostListParams {
    public let page: UInt32 = 1
    public let perPage: UInt32 = 10
}

I was thinking a bit more about this, and I think it might be possible to handle it in a different way. [...] call PostListParams.default() - but I'd have to check.

I'd say the current generated code looks pretty good. It'd be nice to have nil value (hopefully we don't have wait too long), but figuring out the default value at the server side and assigning a default value here in the library seems fine to me.

In Swift, there is rarely a default static function to create an plain object type (or value type). Because there is other features like default parameter values.

@oguzkocer
Copy link
Contributor Author

In Swift, there is rarely a default static function to create an plain object type (or value type). Because there is other features like default parameter values.

That's how Kotlin works as well. I could see a future where uniffi interprets the Default trait in Rust and convert that to the default values for the Swift/Kotlin constructors while generating the bindings.

Although this is probably not too difficult to implement - of course without knowing the internals it's hard to say - I assume there are more important things to improve in the uniffi library at this point.

Another alternative is to generate these bindings ourselves. Now that I have some experience of that with wp_derive, I don't think it'd be too difficult to do.

In any case, I am happy that we have multiple ways to resolve these minor issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants