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.

Sign up for GitHub

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

Jump to bottom

Feature request: Easier way to output typescript optionals, IE ?: Thing for rust Option types, rather than Thing | null #364

Open
dessalines opened this issue Nov 2, 2024 · 7 comments · May be fixed by #366
Open

Feature request: Easier way to output typescript optionals, IE ?: Thing for rust Option types, rather than Thing | null #364

dessalines opened this issue Nov 2, 2024 · 7 comments · May be fixed by #366
Labels
breaking change This PR will change the public API in some way enhancement New feature or request

Comments

@dessalines
Copy link
Contributor

dessalines commented Nov 2, 2024 •
edited
Loading

Is your feature request related to a problem? Please describe.

We're upgradiing our ts-rs from 7.1.1 -> 10.0.0, but unfortunately this upgrade broke all of our Option types.

Beforehand, we could do this on a struct:

#[skip_serializing_none]
#[cfg_attr(feature = "full", ts(export))]
pub struct AdminPurgePersonView {
  admin: Option<Person>
}

And this would generate:

export interface AdminPurgePersonView {
  admin?: Person

With the upgrade, the above now generates:

admin: Person | null

This must be because ts-rs is now ignoring serde's skip_serializing_none directive.

Describe the solution you'd like

Have ts-rs follow the above directive.

Describe alternatives you've considered

  • Add ts(optional) to every single field, but this would be tedious and verbose.
  • Change the default behavior of rust's options to match actual generated typescript optionals (IE ?) , rather than outputting thing | null, which isn't the preferred way typescript deals with optionals.

You don't need to get to into it if you don't want, but what's the short version for why ts-rs outputs thing | null rather than proper typescript optionals?
@dessalines dessalines added the enhancement New feature or request label Nov 2, 2024
@dessalines dessalines changed the title Feature request: Add support for serde skip_serializing_none Feature request: Add support for serde skip_serializing_none for Option types. Nov 2, 2024
@dessalines dessalines changed the title Feature request: Add support for serde skip_serializing_none for Option types. Feature request: Add support for serde skip_serializing_none for Option types, or output proper typescript optionals. Nov 2, 2024
@dessalines dessalines changed the title Feature request: Add support for serde skip_serializing_none for Option types, or output proper typescript optionals. Feature request: Add support for serde skip_serializing_none for Option types, or output proper typescript optionals by default. Nov 2, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Nov 2, 2024 •
edited
Loading

Hey, thanks for opening the issue.
We stopped using #[serde(skip_serializing)], #[serde(skip_deserializing)], #[skip_serializing_if = "..")] (serde_with generates that) in v8 iirc.
The reason for that change was that there's no one right thing we could do - If users only deserialize their types, then we should ignore #[skip_serializing_if = "..")]. If users only serialize their types, then #[skip_serializing_if = "..")] should act as #[ts(optional)]. We just dont know for which "direction" the TS bindings will be used, so we can't really use these "one-directional" serde attributes by default.

Right now, there's no convenient alternative to #[ts(optional)], but I'm certainly open to adding something to make it less cumbersome.

@dessalines
Copy link
Contributor Author

That's reasonable. I can start to add those ts(optional) directives then.

One other question: is there a reason why thing | null is generated rather than ? . Mainly because ? seems to be the preferred way for typescript to handle optionals: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#optional-properties

@NyxCode
Copy link
Collaborator

NyxCode commented Nov 2, 2024

We chose this representation since it better matches what serde_json would produce - a value for Some, and null for None. If we chose ? instead, it would be technically incompatible, since you can't assign null to ?.

@dessalines
Copy link
Contributor Author

Ah so its matching serde. Gotcha, thx!

@dessalines dessalines closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2024
@dessalines dessalines mentioned this issue Nov 2, 2024
Merged
@Nutomic
Copy link

Nutomic commented Nov 6, 2024

@dessalines Can you reopen this issue? Maybe in the future there can be a better solution, because right now its extremely verbose (see LemmyNet/lemmy#5163).

@dessalines
Copy link
Contributor Author

Sure, maybe the maintainers here could think of a good solution, ideally one that could be applied at the struct level.

@dessalines dessalines reopened this Nov 6, 2024
@dessalines dessalines changed the title Feature request: Add support for serde skip_serializing_none for Option types, or output proper typescript optionals by default. Feature request: Easier way to output typescript optionals, IE ?: Thing for rust Option types, rather than Thing | null Nov 6, 2024
@gustavo-shigueo
Copy link
Collaborator

I suppose we could make #[ts(optional)] work at the struct level. What do you think @NyxCode?

@gustavo-shigueo gustavo-shigueo linked a pull request Nov 9, 2024 that will close this issue
Open
3 tasks
@gustavo-shigueo gustavo-shigueo added the breaking change This PR will change the public API in some way label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking change This PR will change the public API in some way enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add #[ts(optional)] to struct Aleph-Alpha/ts-rs
4 participants
@dessalines @Nutomic @NyxCode @gustavo-shigueo