Aleph-Alpha / ts-rs

Generate TypeScript bindings from Rust types
MIT License
1.15k stars 115 forks source link

Optional fields with `#[serde(default)]` for deserialization #175

Closed BenJeau closed 9 months ago

BenJeau commented 1 year ago

Hi thanks for the library!

For a struct with serde default for some fields, the fields becomes optional when Deserializing and if not define will use the default value. I think the resulting TypeScript code should reflect that. The following Rust code:

#[derive(Deserialize, TS)]
#[ts(export)]
pub struct Request {
    url: String,
    #[serde(default)]
    method: Method,
    #[serde(default = "default_timeout")]
    timeout: u32,
}

#[derive(Deserialize, TS, Default)]
#[ts(export)]
enum Method {
    #[default]
    Get,
    Post,
    Put,
    Delete,
}

fn default_timeout() -> u32  {
    10_000
}

Should result in the following TypeScript types if we only care about deserialization

interface Request {
    url: string;
    method?: Method;
    timeout?: number;
}
escritorio-gustavo commented 9 months ago

Use #[ts(optional)]

abhay-agarwal commented 8 months ago

For individual fields labeled as #[serde(default)], ts_rs could label the fields as optional. Currently a #[ts(optional)] marker only works on Option<> types, though. This would help me specifically with boolean types which are often given as optional types and only serialized if true.

escritorio-gustavo commented 8 months ago

For individual fields labeled as #[serde(default)], ts_rs could label the fields as optional. Currently a #[ts(optional)] marker only works on Option<> types, though. This would help me specifically with boolean types which are often given as optional types and only serialized if true.

You could do it like this:

use serde::Serialize;

#[derive(TS, Serialize)]
#[ts(export)]
struct Foo {
    #[ts(optional)]
    #[serde(skip_serializing_if = "should_skip", default = "Default::default")]
    my_optional_bool: Option<bool>,
}

fn should_skip(field: &Option<bool>) -> bool {
    matches!(field, None | Some(false))
}
abhay-agarwal commented 8 months ago

Right, this is what we were doing before. However, because Rust doesn't have a True object, our code was required to "know" that the Option was always Option(true) or None, but never Option(false), and sometimes we would unwittingly add Option(false) in there if, for example, option chaining wasn't correctly written. So my preference would be to allow ts_rs to generate the question mark property if there was a serde(default), as that should in theory allow for the property to be declared optional.

escritorio-gustavo commented 7 months ago

I think this is data modeling error. You're saying your data should only have two valid states, but it has three. Something more appropriate would be using Option<()> and implementing (De)Serialize to convert it to ?: boolean:

use ts_rs::TS;
use serde::{Deserialize, Serialize};

#[derive(TS, Serialize, Deserialize, PartialEq, Debug)]
#[ts(export)]
struct Foo {
    #[ts(optional, as = "Option<bool>")]
    #[serde(skip_serializing_if = "Option::is_none", default = "Default::default", with = "deser")]
    my_optional_bool: Option<()>,
}

mod deser {
    use serde::{Serializer, Serialize, Deserializer, Deserialize};

    pub fn serialize<S: Serializer>(value: &Option<()>, serializer: S) -> Result<S::Ok, S::Error> {
        value.map(|_| true).serialize(serializer)
    }

    pub fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> Result<Option<()>, D::Error> {
        Ok(Option::<bool>::deserialize(deserializer)?.filter(|&x| x).map(|_| ()))
    }
}

#[test]
fn test() {
    let none = Foo { my_optional_bool: None };
    let some = Foo { my_optional_bool: Some(()) };

    // Type definition
    assert_eq!(Foo::inline(), "{ my_optional_bool?: boolean, }");

    // Serializing
    assert_eq!(serde_json::to_string(&none).unwrap(), "{}");
    assert_eq!(serde_json::to_string(&some).unwrap(), r#"{"my_optional_bool":true}"#);

    // Deserializing
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":true}"#).unwrap(), some);
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":false}"#).unwrap(), none); // `false` becomes `None`!
    assert_eq!(serde_json::from_str::<Foo>("{}").unwrap(), none);
}
escritorio-gustavo commented 7 months ago

If you really want to use Option<bool>, then your code should handle Some(false), because it is a valid value for that type, and therefore, sooner or later, it will show up in your codebase. You can't just go "Eh, it'll never happen", because as you said, it already does. Make your invalid states unrepresentable with Option<()> or handle all representable states appropriately with something like

match optional_bool {
    Some(true) => todo!("True case"),
    Some(false) | None => todo!("False case")
}

or

if optional_bool.filter(|&x| x).is_some() {
    todo!("True case"),
} else {
    todo!("False case")
}

or something else that handles all three cases

abhay-agarwal commented 7 months ago

My usecase is one where nonexistence of a value is equivalent to passing that value as false. Serde allows that using the default macro. In our code, we treat the value as boolean because there is either true or [false/undefined]. We don’t want to use Option for the very reason you described — it creates a third state in the data model.

Unfortunately, ts_rs only allows for optional fields to be declared as Option. If Rust had a “true” object, then we could do Option or just Option<()> however I prefer that our rust code use boolean rather than Option<()>.

For me, it makes complete sense for a non-Option field with #[serde(default)] to be rendered as “field?: type” in ts. I would support a feature that made that conversion possible.

NyxCode commented 7 months ago

For me, it makes complete sense for a non-Option field with #[serde(default)] to be rendered as “field?: type” in ts. I would support a feature that made that conversion possible.

We've had something somewhat similar in a previous version, but ended up removing it.
The problem is that we don't know what you're going to do with your types - If you're sending them to a rust server, for example, then yeah, #[serde(default)] should be field?: Type, but if they represent a server response, then they should be field: Type.

Like #[serde(default)], there are a lot of "asymmetric" serde attributes we don't support, like skip_serializing_if, skip_serializing and skip_deserializing. Whichever way we handeled them, it'd be wrong in 50% of the cases.

So yeah, this all gets kinda tricky.
Whether we can make #[ts(optional)] work on non-Option fields might be worth investigating, but I'm afraid we'll hit lots of edge-cases doing that. Maybe just doing #[ts(type = "number | undefined")] is good enough?

abhay-agarwal commented 7 months ago

In my mind ts_rs is a code generator, but doesn’t explicitly guarantee that the generated data is consistent or logical. So for our case, generating an optional parameter with a question mark should be something the user can do at their own behest. In typescript, a parameter that is declared optional can be removed from the instantiation, but one declared type | optional still needs to be instantiated.

I think a ts(optional) would be great that supports arbitrary objects. And yes, if we were to be fully bidirectionally consistent then we would need to use serde(default) and skip_serializing_if(x). But that’s our own responsibility IMO.

NyxCode commented 7 months ago

Fair enough! I've noted this in #294.
Some thought needs to be put into this to figure out if

abhay-agarwal commented 7 months ago

Given that ts optional is only available for Option types, this won’t be a breaking release if the functionality is kept as is for option types. In fact it should basically “just work” if you remove the requirement for Option in the macro. Can submit a PR if helpful

NyxCode commented 7 months ago

I think it's important that #[ts(optional)] behaves predictably, so using it on Option<T> and T should do the same.
However, on Option<T>, it generates field?: T.

Only #[ts(optional = nullable)] generates field?: T | null. That's the behaviour we'd want when used on a non-Option type - keep the type the same, and add a ? to the field.

So right now, we could support #[ts(optional = nullable)] on every type, while disallowing #[ts(optional)].
Or we accept that they behave differently, but I'm really not sure about that.

escritorio-gustavo commented 7 months ago

I prefer that our rust code use boolean rather than Option<()>.

Then this becomes even simpler, no custom (de)serialization required:

use ts_rs::TS;
use serde::{Deserialize, Serialize};

#[derive(TS, Serialize, Deserialize, PartialEq, Debug)]
#[ts(export)]
struct Foo {
    #[ts(optional, as = "Option<bool>")]
    #[serde(skip_serializing_if = "std::ops::Not::not", default = "Default::default")]
    my_optional_bool: bool,
}

#[test]
fn test() {
    let falsy = Foo { my_optional_bool: false };
    let truthy = Foo { my_optional_bool: true };

    // Type definition
    assert_eq!(Foo::inline(), "{ my_optional_bool?: boolean, }");

    // Serializing
    assert_eq!(serde_json::to_string(&falsy).unwrap(), "{}"); // `false` is not serialized
    assert_eq!(serde_json::to_string(&truthy).unwrap(), r#"{"my_optional_bool":true}"#);

    // Deserializing
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":true}"#).unwrap(), truthy);
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":false}"#).unwrap(), falsy);
    assert_eq!(serde_json::from_str::<Foo>("{}").unwrap(), falsy); // `undefined` is deserialized into `false`
}
escritorio-gustavo commented 7 months ago

With #[ts(as = "...")] you can just make TS interpret your type as if it were Option<T> instead of T, and then add #[ts(optional)] to change the type to field?: T instead of field: T | null.

The feature you're asking for already exists through as + optional

NyxCode commented 7 months ago

That's a very nice solution! That didn't even cross my mind.

abhay-agarwal commented 7 months ago

This is a good intermediate solution for sure! It would be nice to have a solution that can use the type as defined, so that there's no duplication between as = Option<X> and field: X

But I appreciate that recommendation, I can implement that solution now.

abhay-agarwal commented 7 months ago

As a real example, here's my (now updated) definition of a struct:

#[derive(Debug, Deserialize, Serialize, TS, Clone, PartialEq)]
#[serde(rename_all = "camelCase")]
#[ts(export, export_to = "ts/")]
pub struct FilePropertySelection {
    #[ts(optional, as = "Option<bool>")]
    #[serde(default, skip_serializing_if = "std::ops::Not::not")]
    pub everything: bool,
    /// Return all properties within the given groups (along with properties)
    #[ts(optional, as = "Option<HashSet<EntityPropertyGroupId>>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub groups: HashSet<EntityPropertyGroupId>,
    /// Return all given properties (along with groups)
    /// WARN: If a property is specified, and it doesn't exist, and error will
    /// occur
    #[ts(optional, as = "Option<HashSet<FilePropertyId>>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub properties: HashSet<FilePropertyId>,
}

As you can see, the use of as = ... does work, it just doesn't quite get me to the point where I feel like the problem is "solved". I would love to not have to use the duplicative syntax if possible. But again, this is a choice for your side, as I can technically achieve what I want.

escritorio-gustavo commented 7 months ago

I would love to not have to use the duplicative syntax if possible.

We could allow as to parse Option<_> and have _ be converted into the field's original type

use ts_rs::TS;
use serde::{Deserialize, Serialize};

#[derive(TS, Serialize, Deserialize, PartialEq, Debug)]
#[ts(export)]
struct Foo {
    #[ts(optional, as = "Option<_>")]
    #[serde(skip_serializing_if = "std::ops::Not::not", default)]
    my_optional_bool: bool,
}

#[test]
fn test() {
    let falsy = Foo { my_optional_bool: false };
    let truthy = Foo { my_optional_bool: true };

    // Type definition
    assert_eq!(Foo::inline(), "{ my_optional_bool?: boolean, }");

    // Serializing
    assert_eq!(serde_json::to_string(&falsy).unwrap(), "{}"); // `false` is not serialized
    assert_eq!(serde_json::to_string(&truthy).unwrap(), r#"{"my_optional_bool":true}"#);

    // Deserializing
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":true}"#).unwrap(), truthy);
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":false}"#).unwrap(), falsy);
    assert_eq!(serde_json::from_str::<Foo>("{}").unwrap(), falsy); // `undefined` is deserialized into `false`
}
escritorio-gustavo commented 7 months ago

Your example would then become

#[derive(Debug, Deserialize, Serialize, TS, Clone, PartialEq)]
#[serde(rename_all = "camelCase")]
#[ts(export, export_to = "ts/")]
pub struct FilePropertySelection {
    #[ts(optional, as = "Option<_>")]
    #[serde(default, skip_serializing_if = "std::ops::Not::not")]
    pub everything: bool,
    /// Return all properties within the given groups (along with properties)
    #[ts(optional, as = "Option<_>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub groups: HashSet<EntityPropertyGroupId>,
    /// Return all given properties (along with groups)
    /// WARN: If a property is specified, and it doesn't exist, and error will
    /// occur
    #[ts(optional, as = "Option<_>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub properties: HashSet<FilePropertyId>,
}
abhay-agarwal commented 7 months ago

Sounds great!

abhay-agarwal commented 7 months ago

I still think the best/easiest solution is to just allow for the #[ts(optional)] to be used on non-option types, but your solution is appropriate as well.

gustavo-shigueo commented 7 months ago

Great! I have already coded it on my work computer and will submit a PR on monday

escritorio-gustavo commented 7 months ago

Check out #299