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

Add #[ts(optional)] to struct #366

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

gustavo-shigueo
Copy link
Collaborator

Goal

Allow the use of #[ts(optional)] on struct definitions
Closes #364

Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

@gustavo-shigueo gustavo-shigueo added the enhancement New feature or request label Nov 9, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Nov 9, 2024

Love the idea. Would also like to hear what @Nutomic and @dessalines think.

I'm not yet sure if re-using the same #[ts(optional)] on a struct is a good idea syntax-wise, but maybe it'll grow on me.

@dessalines
Copy link
Contributor

Excellent! Yep this would really gonna clean up our structs. Thx!

@NyxCode
Copy link
Collaborator

NyxCode commented Nov 10, 2024

@gustavo-shigueo A problem I see with this implementation is that all fields have to be Option<T>, otherwise this error is emitted: error: 'optional' can only be used on an Option<T> type.

We could fix that by trying to figure out if a field is an Option, and if it isn't, using the default behaviour. But like the current handling of #[ts(optional)], that's fragile, and breaks with type aliases.

Maybe we can move that logic from the macro to the runtime, where we actually know if a type is an Option or not.

@gustavo-shigueo
Copy link
Collaborator Author

@gustavo-shigueo A problem I see with this implementation is that all fields have to be Option<T>, otherwise this error is emitted: error: 'optional' can only be used on an Option<T> type.

Damn, I forgot about that

We could fix that by trying to figure out if a field is an Option, and if it isn't, using the default behaviour. But like the current handling of #[ts(optional)], that's fragile, and breaks with type aliases.

Yeah, that would probably not be a great idea

@gustavo-shigueo
Copy link
Collaborator Author

I implemented a band-aid fix for the problem, but we should probably look into a better way to detect the Option type

@gustavo-shigueo
Copy link
Collaborator Author

Maybe we can move that logic from the macro to the runtime, where we actually know if a type is an Option or not.

We could add a const IS_OPTION: bool = false to the TS trait and set it to true in our impl<T: TS> TS for Option<T>. That shouldn't even be a breaking change since the trait provides a default value

@gustavo-shigueo
Copy link
Collaborator Author

@NyxCode I have implemented this idea for you to take a look, I also change Optional into an enum, this way it is impossible to represent the invalid Optional { optional: false, nullable: true }

@gustavo-shigueo
Copy link
Collaborator Author

gustavo-shigueo commented Nov 10, 2024

The downside of this implementation is that there is no compile time error for

#[derive(TS)]
struct Foo {
    #[ts(optional)]
    foo: i32
}

Instead, this will panic when the user attempts to export their types. I don't quite like this because ts-rs usually works in a "If it compiles, you're good" way, but here we get something that compiles and doesn't work.

@NyxCode
Copy link
Collaborator

NyxCode commented Nov 10, 2024

Nice work! I have an idea how to get this compile-time checked, lemme see if that actually works though..

Comment on lines 119 to 128
let optional_annotation = if let Optional::Optional { .. } = field_attr.optional {
quote! {
if <#ty as #crate_rename::TS>::IS_OPTION {
#optional_annotation
} else {
panic!("`#[ts(optional)]` can only be used with the Option type")
}
}
Optional {
optional: false, ..
} => (&parsed_ty, ""),
} else {
optional_annotation
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is just remove this whole thing, this way there'll be no panic (still, no compiler error). What will happen is that #[ts(optional)] will just do nothing when not using an Option type

@gustavo-shigueo
Copy link
Collaborator Author

I'm not yet sure if re-using the same #[ts(optional)] on a struct is a good idea syntax-wise, but maybe it'll grow on me.

Good point, maybe at the struct level we should call it something like optional_fields

@gustavo-shigueo
Copy link
Collaborator Author

maybe at the struct level we should call it something like optional_fields

@NyxCode I've applied this change, but if you think of a better name, feel free to push a commit here

@gustavo-shigueo gustavo-shigueo added the breaking change This PR will change the public API in some way label Nov 11, 2024
@gustavo-shigueo
Copy link
Collaborator Author

I also added the breaking change tag because this will break manual implementations of TS. As always, the feature we need to avoid that is unstable lol (rust-lang/rust#29661)

@NyxCode
Copy link
Collaborator

NyxCode commented Dec 1, 2024

I think I'm happy with the new name - nice!
Besides docs, is there anything we need to get this merged?

@gustavo-shigueo
Copy link
Collaborator Author

Besides docs, is there anything we need to get this merged?

No, I think it's just the docs

@NyxCode
Copy link
Collaborator

NyxCode commented Dec 2, 2024

Should we infer #[ts(optional)] when #[serde(skip_serializing_if = "Option::is_none")] and #[serde(default)] is present?
I believe that would be correct, though possibly somewhat unpredictable, so I'm not sure if that's a good idea.

Edit: Never mind, I dont think this is a good idea.

@NyxCode
Copy link
Collaborator

NyxCode commented Dec 2, 2024

What's the expected behavior with #[ts(optional_fields)] on a struct and #[ts(type)] on a field?

#[ts(optional_fields)]
struct X {
  a: Option<i32>, // a?: number
  #[ts(type = "string")]
  b: Option<SomethingElse>,
}

Right now, the #[ts(type = "..")] works as expected - but we still get the ? for b: b?: string.
I do suspect that #[ts(type)] should override that, resulting in just b: string.

@gustavo-shigueo
Copy link
Collaborator Author

gustavo-shigueo commented Dec 2, 2024

What's the expected behavior with #[ts(optional_fields)] on a struct and #[ts(type)] on a field?

#[ts(optional_fields)]
struct X {
  a: Option<i32>, // a?: number
  #[ts(type = "string")]
  b: Option<SomethingElse>,
}

Right now, the #[ts(type = "..")] works as expected - but we still get the ? for b: b?: string. I do suspect that #[ts(type)] should override that, resulting in just b: string.

Nice catch! I've changed format_field to handle #[ts(type = "...")] in isolation from other attibutes (except for rename and rename_all), I also made it so you cannot use #[ts(type = "...")] and #[ts(optional)] on the same field, as #[ts(optional)] would have no effect

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR will change the public API in some way enhancement New feature or request
Projects
None yet
3 participants