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

Support #[serde(with = "...")] for struct fields #280

Merged
merged 10 commits into from
Apr 9, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Mar 21, 2024

Goal

Ensure the #[serde(with = "...")] attribute is properly handled in a way that prevents users from accidentally generating wrong TS type definitions by forgetting to add #[ts(as = "...")]/#[ts(type = "...")],

Why?

The #[serde(with = "...")] attribute changes how the type is serialized, therefore, it changes what type will be received by TypeScript, so the type generated by default will most likely be incorrect. The user must use #[ts(as = "...")] or #[ts(type = "...")] to ensure their type definition matches the type generated by serde

Why not make #[serde(with = "...")] an alias for #[ts(as = "...")]

#[ts(as = "...")] only accepts types, where as #[serde(with = "...")] accepts either a type or a path to a module containing both a serialize and a deserialize function, so the attributes are not compatible and thus cannot just alias each other

Closes #275

Changes

When the serde-compat feature is enabled, using #[serde(with = "...")] will demand the use of either #[ts(as = "...")] or #[ts(type = "...")], resulting in a compiler error if neither is used

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.

Todo

  • #[serde(with = "...")] is a valid variant attribute, but I'm not yet clear on how to implement it, as neither #[ts(as = "...")] or #[ts(type = "...")] are valid variant attributes.
  • Find a way to test the compiler error case (Maybe compiletest_rs or trybuild?)

@escritorio-gustavo escritorio-gustavo changed the title Support serde with Support #[serde(with = "...")] Mar 21, 2024
@escritorio-gustavo escritorio-gustavo changed the title Support #[serde(with = "...")] Support #[serde(with = "...")] for struct fields Mar 21, 2024
@NyxCode NyxCode mentioned this pull request Mar 22, 2024
3 tasks
@NyxCode
Copy link
Collaborator

NyxCode commented Mar 22, 2024

Huh, this is much less complicated than I thought it might end up being, nice!

#[serde(with = "...")] is a valid variant attribute, but I'm not yet clear on how to implement it, as neither #[ts(as = "...")] or #[ts(type = "...")] are valid variant attributes.

Hm. I guess we could at support for them for variants, or leave this edge-case for now. Will have to look into how complicated supporting these attributes for variants would be first, though I think it might be pretty straight forward.

Find a way to test the compiler error case (Maybe compiletest_rs or trybuild?)

I think that's supported in doctests with "```compile_fail ....`". Not super clean, but it's simple, so maybe that's enough?

@escritorio-gustavo
Copy link
Contributor Author

I think that's supported in doctests with "```compile_fail ....`". Not super clean, but it's simple, so maybe that's enough?

It is, but I'm not sure if a doctest in the tests directory would be executed by cargo test

@escritorio-gustavo
Copy link
Contributor Author

Hm. I guess we could at support for them for variants, or leave this edge-case for now. Will have to look into how complicated supporting these attributes for variants would be first, though I think it might be pretty straight forward.

That might require two new PRs, one for #[ts(as = "...")]/#[ts(type = "...")] and another for #[serde(with = "...")]. Otherwise I think this one would get too large

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 22, 2024

Agreed!

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.

Didn't find the time to take a closer look earlier, sorry about that.
Everything looks good to me here! I was still wishing that we could actually support #[serde(with = "..")], but this is still better than the status quo and is actually robust. I'm sold!

@escritorio-gustavo escritorio-gustavo merged commit ff93760 into main Apr 9, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the support_serde_with branch April 9, 2024 11:45
@escritorio-gustavo escritorio-gustavo added the breaking change This PR will change the public API in some way label Apr 17, 2024
@LZQCN
Copy link

LZQCN commented Jun 4, 2024

Good PR, Please release a new version that includes these updates. I would really like to use them in my project. Thank you!

@escritorio-gustavo
Copy link
Contributor Author

Hello @LZQCN! These changes will eventually be released with version 9 of the library, but we still need to complete #284, which covers this same feature on enum variants. It hasn't been merged yet because I couldn't quite figure out a way to test it and compare the results with serde's output.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we support #[serde(with = "..")]?
3 participants