Skip to content

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

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

Merged
merged 27 commits into from
Feb 21, 2025
Merged

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 gustavo-shigueo linked an issue Feb 20, 2025 that may be closed by this pull request
Copy link
Collaborator

@NyxCode NyxCode left a comment

Choose a reason for hiding this comment

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

Hey @gustavo-shigueo! I am - and have been - very busy lately. Sorry for being less active here.
Thank you for keeping an eye on everything!

Nonetheless, I'd like to get this merged, and release a new version in a new PR.

The only thing missing here is a note in the changelog, I think. Is there something else you'd like to change/note/add?

@gustavo-shigueo
Copy link
Collaborator Author

Hey @gustavo-shigueo! I am - and have been - very busy lately. Sorry for being less active here.
Thank you for keeping an eye on everything!

Hey @NyxCode, I hope you're doing well! No problem, I've also been pretty busy lately, I've been keeping an eye on the issues but haven't written any code in a while.

Nonetheless, I'd like to get this merged, and release a new version in a new PR.

The only thing missing here is a note in the changelog, I think.

Awesome! I'll make sure to add it tomorrow morning, thanks!

@gustavo-shigueo
Copy link
Collaborator Author

Updated the changelog, I also ran cargo msrv verify to check if our rust-version was correct and it turns out the MSRV is now 1.78.0

@gustavo-shigueo
Copy link
Collaborator Author

Updated the changelog, I also ran cargo msrv verify to check if our rust-version was correct and it turns out the MSRV is now 1.78.0

I'll see if I can make a GH workflow this weekend to check that for us in the future

@gustavo-shigueo gustavo-shigueo merged commit ad0f229 into main Feb 21, 2025
18 checks passed
@gustavo-shigueo gustavo-shigueo deleted the add_ts_optional_to_struct branch February 21, 2025 12:19
@gustavo-shigueo gustavo-shigueo mentioned this pull request Feb 22, 2025
3 tasks
@dessalines
Copy link
Contributor

Would it be possible to do a minor release sometime soon (that has this)?

@gustavo-shigueo
Copy link
Collaborator Author

This is a breaking change for anyone manually implementing TS, so this must be in a major release

@gustavo-shigueo
Copy link
Collaborator Author

gustavo-shigueo commented Apr 16, 2025

I think it could be possible to have a major release about now, the only PR that I think is worth looking into before hand is #376, which is waiting for @NyxCode's review, and maaaaybe we could look into having a pre-release version of the CLI, but that's unlikely, due to the concerns raised in #382 and the fact that I'm currently unable to push commits to the CLI branch

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 19, 2025

@gustavo-shigueo I've raised the permissions issue with the org. They reshuffled the management of it, but I hope they'll finally get around to it after the easter holidays. Again, sorry about that.

I'll merge the deno PR & cut a release early next week.

@gustavo-shigueo
Copy link
Collaborator Author

@gustavo-shigueo I've raised the permissions issue with the org. They reshuffled the management of it, but I hope they'll finally get around to it after the easter holidays. Again, sorry about that.

Thanks for looking into it! No need to apologize, though. This isn't on you at all

I'll merge the deno PR & cut a release early next week.

Awesome!

@gustavo-shigueo
Copy link
Collaborator Author

I hope they'll finally get around to it after the easter holidays

Hey @NyxCode, just checking if have you've heard anything from them

@NyxCode
Copy link
Collaborator

NyxCode commented May 29, 2025

Hey @gustavo-shigueo!
I've just checked again, and I think they've fixed it! You should have gotten an invitation, probably per email?

@gustavo-shigueo
Copy link
Collaborator Author

Hey @NyxCode, yeah, I just received it, thanks!

@NyxCode
Copy link
Collaborator

NyxCode commented May 29, 2025

Yay! Man, that was a journey. Sorry that it took so long.

@gustavo-shigueo
Copy link
Collaborator Author

Don't worry about it

@dessalines
Copy link
Contributor

Happy to say we'll be using this feature soon in lemmy: LemmyNet/lemmy#5777

Thx again!

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