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

Hackily support non-'static lifetimes #117

Closed
wants to merge 4 commits into from
Closed

Conversation

petersn
Copy link

@petersn petersn commented Sep 14, 2022

This is a hacky attempt to make ts-rs support non-'static lifetimes by simply dropping the quantification over lifetimes in our impls of TS, and remapping every lifetime referenced inside of the impl to be 'static. This PR seems to pass the tests, but I'll admit that I just hacked this together in a couple hours without carefully looking over ts-rs, so I have no idea if it breaks everything. Almost certainly this deserves more careful scrutiny by somebody who didn't just come to ts-rs's source for the first time today.

Here is an example struct that didn't work before, but works now:

#[derive(ts_rs::TS)]
#[ts(export)]
struct Foo<'a> {
    field: &'a str,
}

(This correctly exports to export interface Foo<> { field: string, }.)

@NyxCode
Copy link
Collaborator

NyxCode commented Sep 17, 2022

Thanks for your contribution, I really appreciate it. I will take a closer look in the coming days.

@NyxCode NyxCode linked an issue Sep 17, 2022 that may be closed by this pull request
@petersn
Copy link
Author

petersn commented Sep 17, 2022

I have four other changes on my branch:

  1. I also made lifetimes work in anonymous fields of structs -- I should probably cherry-pick that commit into this PR.
  2. I made slices turn into Arrays, because currently a field like &'a [i32] doesn't work.
  3. Currently ts-rs doesn't handle rename_all attributes applied to enum variants, resulting in the TypeScript it outputs being incompatible with what Serde outputs. I have a fix for this.
  4. Currently skip_serializing_if only works if the argument is Option::is_none, and the TypeScript unwraps the Option, to avoid also having a | null. I made it emit a ? TypeScript field if it's something else that's unknown. I'm not totally clear on what the right behavior is here.
    I should probably make another PR, or make several, I assume?

@thomasmost
Copy link

@NyxCode Just wanted to +1 this, if you have bandwidth!

@petersn
Copy link
Author

petersn commented Feb 6, 2023

@thomasmost In case you're trying to use this right now, you can also include in your Cargo.toml via:

ts-rs = { git = "https://github.com/petersn/ts-rs", branch = "optional-changes" }

This should give you all of the above mentioned features (non-static lifetimes, slices becoming arrays, rename_all on enum variants, and skip_serializing_if behavior that's more consistent with serde).

@petersn
Copy link
Author

petersn commented Aug 22, 2023

@NyxCode Bump on this? I think the full set of additions in my optional-changes branch (listed above) are actually pretty good, and should probably be incorporated if they haven't already been implemented.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Jan 22, 2024

I believe #128 (and possibly other PRs) have fixed this, as the example provided no longer requires 'a: 'static (see this test)

Slices now work as the PRs #183 and #199 were directed at fixing them

rename_all has worked on enum variants for some time, though I don't know when that was fixed (see #135)

I haven't checked the skip_serializing_if behavior, but I do believe serde converts None to null, so | null is likely correct behavior

Edit: rename_all on enums was fixed in #122

@escritorio-gustavo
Copy link
Contributor

I haven't checked the skip_serializing_if behavior, but I do believe serde converts None to null, so | null is likely correct behavior

skip_serializing_if is planned to be completely ignored in future versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't require 'static
4 participants