Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
NyxCode opened this issue Mar 19, 2024 · 11 comments · Fixed by #280
Closed

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

NyxCode opened this issue Mar 19, 2024 · 11 comments · Fixed by #280
Labels
enhancement New feature or request question Further information is requested

Comments

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 19, 2024

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

@NyxCode NyxCode added enhancement New feature or request question Further information is requested labels Mar 19, 2024
@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 20, 2024

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
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

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

@escritorio-gustavo
Copy link
Contributor

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
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 20, 2024

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
Copy link
Contributor

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

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

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
Copy link
Contributor

escritorio-gustavo commented Mar 20, 2024

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants