Optional Options should have a null type

[ryanw]

Currently this:

struct Foo {
  bar: Option<String>
}

Will generate:

interface Foo {
  bar: string | null,
}

But this:

struct Foo {
  #[ts(optional)]
  bar: Option<String>
}

Will generate:

interface Foo {
  bar?: string,
}

This means it's not possible to assign the required value to the #[ts(optional)] one as you end up with the error:

Type 'null' is not assignable to type 'string | undefined'

It seems to be me that the #[ts(optional)] attribute shouldn't change the type at all, and simply adds the ? to the field. i.e. It should spit out:

interface Foo {
  bar?: string | null,
}

[petersn]

I agree that | null and ? are sort of orthogonal, and probably there should be finer grained control here. Analogously, currently #[serde(skip_serializing_if = "...")] only checks for Option::is_none, because ts-rs's "optional" field simply means replacing an Option<T> field with a T field and slapping a ? on it. This means that ts-rs doesn't emit ?s for other skip_serializing_if fields, because it can't know that they're on Option types.

I propose we replace this with two concepts: optional (?), and nullable (| null). By default Option<T> produces a nullable field, and skip_serializing_if always produces an optional field. We can even make it so that if it detects skip_serializing_if = "Option::is_none" that it becomes optional and not nullable, which would make this proposal be a backwards-compatible extension of the current behavior.

I have a patch that does something sort of like this (main...petersn:ts-rs:optional-changes although it's mixed in with other changes), but I'm curious what the maintainers think is the right behavior here.

[escritorio-gustavo]

@NyxCode, what do you think the behavior should be? Should we keep it as is or add an | null to the type?

[NyxCode]

I started writing a reply, and while writing it realized how complex the different tradeoffs are here.
After thinking about this, I think I've come to a conclusion:

Interactions with serde-compat

• serde attributes should get "translated" into #[ts(..)] attributes.
• If, given a set of serde attributes, there are multiple possible types we could generate, I came to the conclusion that we should chose the smallest type (small in a set-theoretical way).
• Finally, that default must be able to be overwritten.

The serde attributes I'm thinking of here are

• #[serde(default)]
• #[serde(skip_serializing_if = "Option::is_none")]

Here is a table to show what effect these attributes have:

Rust type	serializes to	deserializes from	possible TS types
Option<T>	null	null	t: T | null
#[serde(default)] Option<T>	null	null | undefined	t: T | null t?: T | null
#[serde(skip_serializing_if = "is_none")] Option<T>	undefined	null	NONE
#[serde(default)] #[serde(skip_serializing_if = "is_none")] Option<T>	undefined	null | undefined	t?: T t?: T | null

Proposal

We keep #[ts(optional)] as it currently is, but also introduce #[ts(nullable)].
That attribute would be used with #[ts(optional)] to do what is proposed here.

#[serde(default)]

The two possible TS types for this are t: T | null or t?: T | null.
The first one is the smaller type, so it should be the default.
Therefore, we can just ignore the attribute, since t: T | null is what Option<T> get's turned into anyway.

To get the 2nd one, a user can additionally use #[ts(optional, nullable)]

#[serde(skip_serializing_if = "Option::is_none")]

This should result in a warning!
There is no type we could possibly generate that would fit, as far as I can tell.

#[serde(default)] and #[serde(skip_serializing_if = "Option::is_none")]

The two possible TS types for this are t?: T or t?: T | null.
The first one is the smaller type, so it should be the default.
Therefore, these two serde attributes would get translated into #[ts(optional)].

To get the 2nd one, a user can additionally use #[ts(nullable)]

Summary

This is a complicated topic, and I'm still not sure this is the right way.
Any opinions or thoughts on this @escritorio-gustavo ?

[NyxCode]

A smaller sensible change, without introducing #[ts(nullable)], would be to keep everything as-is, but turn #[serde(default)] + #[serde(skip_serializing_if = "Option::is_none")] into #[ts(optional)].

Previously, just #[serde(skip_serializing_if = "Option::is_none")] turned into #[ts(optional)] but we removed that (for good reasons!).

Adding that back in, as long as #[serde(default)] is present, is therefore probably a good idea.

[escritorio-gustavo]

Your smallest possible set idea makes a lot of sense, because it guarantees that the resulting type can always be serialized and deserialized.
Maybe we should force skip_serializing_if to be combined with default through a compile_error? Not sure about that though.

I also think re adding #[serde(skip_serializing_if = "Option::is_none")] to combine with #[default] might be the better way to go

[NyxCode]

Your smallest possible set idea makes a lot of sense, because it guarantees that the resulting type can always be serialized and deserialized.

Not sure if I misread this, or if there was a misunderstanding. But using the "smallest" type is not strictly neccessary. In the table, there are two possible types for e.g #[serde(default)], both of which would serialize and deserialize just fine.
I do however feel like using the "smallest" type is just more precise, and precise = good? 😅

I think however that one motivation of @ryanw to open the issue was that t?: T (which is the "smallest" type for the last table row) is just a bit akward to deal with.
I think he tried to do obj.t = null, but he'd have to do delete obj.t or obj.t = undefined, depending on his usecase.

Maybe we should force skip_serializing_if to be combined with default through a compile_error? Not sure about that though.

I'm sceptical about that. Maybe some users got types which they only deserialize, and never serialize.

I also think re adding #[serde(skip_serializing_if = "Option::is_none")] to combine with #[default] might be the better way to go

I do agree that we probably want to add that back. Though I'd like non-serde-compat mode to be able to do everything that serde-compat mode does, so I think we still need #[ts(nullable)] (or #[ts(non_null)] if we do change #[ts(optional)] as proposed here).

[escritorio-gustavo]

Not sure if I misread this, or if there was a misunderstanding. But using the "smallest" type is not strictly neccessary. In the table, there are two possible types for e.g #[serde(default)], both of which would serialize and deserialize just fine.

I misunderstood, I missed the fact that even if, say, default doesn't serialize to undefined, it's still ok to have it in the TS type

Maybe some users got types which they only deserialize, and never serialize.

That makes sense, let's keep it to a warning

I do agree that we probably want to add that back. Though I'd like non-serde-compat mode to be able to do everything that serde-compat mode does, so I think we still need #[ts(nullable)] (or #[ts(non_null)] if we do change #[ts(optional)] as proposed here).

I see. I think nullable is better than non_null though. Negated names can be confusing, and the whole null vs undefined thing is already confusing enough