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

Implement #[ts(optional = nullable)] #213

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Jan 29, 2024

This PR implements #[ts(optional = nullable)], as discussed in #112.
#[ts(optional)] Option<T> still generates t?: T, while #[ts(optional = nullable)] generates t?: T | null.

I plan on following this up at some point with support for the serde attributes outlined in #112.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Jan 29, 2024

Closes #112

@NyxCode NyxCode marked this pull request as ready for review January 29, 2024 19:25
@NyxCode NyxCode linked an issue Jan 29, 2024 that may be closed by this pull request
@NyxCode
Copy link
Collaborator Author

NyxCode commented Jan 29, 2024

@escritorio-gustavo would love to hear what you think about this!

@escritorio-gustavo
Copy link
Contributor

This is awesome! I thought there would be a separate #[ts(nullable)] attribute, but making it an argument for #[ts(optional)] is way better to prevent it from being used instead of an actual Option

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Jan 30, 2024

Quick question, is the optional field needed? It always true unless the attribute is not present. This could an Option with just one field for nullable

#[derive(default)]
pub struct Nullable(pub bool);

#[derive(Default)]
pub struct FieldAttr {
    pub type_override: Option<String>,
    pub type_override: Option<String>,
    pub rename: Option<String>,
    pub rename: Option<String>,
    pub inline: bool,
    pub inline: bool,
    pub skip: bool,
    pub skip: bool,
    pub optional: bool,
    pub optional: Option<Nullable>,
    pub flatten: bool,
    pub flatten: bool,
}

let (ty, optional_annotation) = match optional {
      Some(Nullable(nullable) => {
          let inner_type = extract_option_argument(&field.ty)?; // inner type of the optional
          match nullable {
              true => (&field.ty, "?"),  // if it's nullable, we keep the original type
              false => (inner_type, "?"), // if not, we use the Option's inner type
          }
      },
      None => (&field.ty, "")
 };

This is just an idea though, I'm totally fine with merging this as is

@NyxCode
Copy link
Collaborator Author

NyxCode commented Jan 30, 2024

Quick question, is the optional field needed? It always true unless the attribute is not present. This could an Option with just one field for nullable

#[derive(default)]
pub struct Nullable(pub bool);

#[derive(Default)]
pub struct FieldAttr {
    pub type_override: Option<String>,
    pub type_override: Option<String>,
    pub rename: Option<String>,
    pub rename: Option<String>,
    pub inline: bool,
    pub inline: bool,
    pub skip: bool,
    pub skip: bool,
    pub optional: bool,
    pub optional: Option<Nullable>,
    pub flatten: bool,
    pub flatten: bool,
}

let (ty, optional_annotation) = match optional {
      Some(Nullable(nullable) => {
          let inner_type = extract_option_argument(&field.ty)?; // inner type of the optional
          match nullable {
              true => (&field.ty, "?"),  // if it's nullable, we keep the original type
              false => (inner_type, "?"), // if not, we use the Option's inner type
          }
      },
      None => (&field.ty, "")
 };

This is just an idea though, I'm totally fine with merging this as is

Sure, that'd work as well. I tried a couple of options, and the one i ended up with felt the least ugly to me.
If you've got strong opinions on this though, feel free to change it.

@escritorio-gustavo
Copy link
Contributor

Sure, that'd work as well. I tried a couple of options, and the one i ended up with felt the least ugly to me.
If you've got strong opinions on this though, feel free to change it.

I don't think there's a need for that, I was just curious

@escritorio-gustavo escritorio-gustavo merged commit 5a5b518 into main Jan 30, 2024
4 checks passed
@escritorio-gustavo escritorio-gustavo deleted the optional-nullable branch January 30, 2024 18:09
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.

Optional Options should have a null type
2 participants