Aleph-Alpha / ts-rs

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

Can we support `#[serde(with = "..")]`? #275

Closed NyxCode closed 7 months ago

NyxCode commented 8 months ago

Would it make sense for us to parse #[serde(with = "..")] as an alias for #[ts(as = "..")]?

serde seems to, when encountering a field with #[serde(with = "X")], (de)serialize with X::(de)serialize.
That means that #[serde(with)] can point to a module with two functions, or two a struct implementing Serialize and Deserialize.

In the 1st case, there's nothing we can do. For case 2, however, we could interpret that as #[ts(as = "X")].

So, to support this, we'd need to figure out if X is a type or a module path. We could decide that based on capitalization (a::b::C is probably a type, and a::b::c is probably a module). If we determine that it's a module, we ignore it. If it's a type, we interpret the annotation as #[ts(as = "..")]. It's not bullet-proof, but probably good enough?

If we went down that road, it'd make sense to have #[ts(type = "..")] or #[ts(as = "..")] take prescedence over #[serde(with = "..")], overriding it.

Happy to hear any input on this!

Links

escritorio-gustavo commented 8 months ago

So, to support this, we'd need to figure out if X is a type or a module path. We could decide that based on capitalization (a::b::C is probably a type, and a::b::c is probably a module).

What if instead of testing capitalization, we generated some code like fn does_this_compile() { <#ty as #crate_rename::TS>::name() }?

NyxCode commented 8 months ago

Does that work? If it doesn't compile, I don't think there's anything we can do about that.

escritorio-gustavo commented 8 months ago

That's what I'm getting at. We shouldn't be the ones deciding that through checking capitalization, we should let the compiler check if whatever the user provided is a type.

NyxCode commented 8 months ago

I agree that that would be peferable, but let's say we generate your does_this_compile, and it doesn't - what now?

escritorio-gustavo commented 8 months ago

We just let the user get the compiler error. It should be something along the lines of "expected a type, got a module" or "type does not implement TS". That should guide the user to fix the attribute

escritorio-gustavo commented 8 months ago

Except this needs to work with serde, which accepts a module there... forgot about that

escritorio-gustavo commented 8 months ago

Still, if we could change the error message to be something like "using #[serde(with = "module")] requires the #[ts(as = "type")] attribute" that would keep serde working and tell the user how to fix the problem. I don't know if that's possible though

escritorio-gustavo commented 8 months ago

Instead of trying to make them compatible, we could make it so using #[serde(with = "..")] demands using #[ts(as = "..")] or #[ts(type = "..")]

NyxCode commented 8 months ago

That'd work! The worst-case there would be that a user had to do

#[ts(as = "TypeA")]
field: TypeA

To be honest, I share your aversion to infering stuff from the path. It does seem unlikely to break, but relying on it is just a bit iffy.

If we're going with the "#[serde(with)] requires a type override" solution, we'll have to consider if the effort/added complexity is worth the small benifit. I'd also be fine with just closing this as wontfix.

escritorio-gustavo commented 8 months ago

That'd work! The worst-case there would be that a user had to do

#[ts(as = "TypeA")]
field: TypeA

I actually think this is good. In general, #[serde(with = "...")] changes the type that TypeScript will receive, so #[ts(as = "...")] should be required. If #[serde(with = "...")] doesn't change the type, the user should explicitly say that like you did in this snippet

To be honest, I share your aversion to infering stuff from the path. It does seem unlikely to break, but relying on it is just a bit iffy.

Yeah, it feels like putting way too much trust in what is, effectively, just a string.

If we're going with the "#[serde(with)] requires a type override" solution, we'll have to consider if the effort/added complexity is worth the small benifit. I'd also be fine with just closing this as wontfix.

Good point, this might actually be difficult to add, so we should be sure it's worth it

escritorio-gustavo commented 7 months ago

That'd work! The worst-case there would be that a user had to do

#[ts(as = "TypeA")]
field: TypeA

With #299 this can be done as

#[ts(as = "_")]
field: TypeA