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

Feature: Add support for types from serde_json #276

Merged
merged 22 commits into from
Mar 20, 2024

Conversation

NyxCode
Copy link
Collaborator

@NyxCode NyxCode commented Mar 19, 2024

Goal

This PR adds support for types from serde_json: serde_json::Value, serde_json::Number and serde_json::Map.

Most common (and most tricky) of them is serde_json::Value.
Right now, the easiest way to get a struct containing a Value to implement TS is to slap a #[ts(type = "any")] on the fields.
However, that's not great, since a JSON object is actually not any.

If a user wanted to get a more precise type for it, they might have tried this:

#[derive(TS)]
#[ts(untagged, export_to = "serde_json/")]
enum TsJsonValue {
    String(String),
    Number(i32),
    Array(Vec<TsJsonValue>),
    Object(HashMap<String, TsJsonValue>),
}

#[derive(TS)]
struct SomeStruct {
    #[ts(as = "TsJsonValue")]
    field: serde_json::Value,
}

Interestingly, tsc doesn't want to compile TsJsonValue.ts. This is one of the (rare) cases where Record<string, TsJsonValue>, which is what we generate, doesn't work, but { [key: string]: TsJsonValue } would.

Changes

In essence, all this PR does is the code above, except that TS had to be implemented manually for serde_json::Value.

Unlike all previous ..-impl features, this one actually exposes an exportable type, JsonValue.
Before, all impls were simply and just generated e.g number. The one for serde_json::Value, however, is recursive, and therefore needs an actual name.

If a user exports a struct containing serde_json::Value, JsonValue.ts will get exported to ./serde_json/.

Alternatives

We could, instead of merging this now, first explore if generating { [key: K]: V } for maps makes sense.
Then, this PR would not require the manual TS implementation (If we made #[derive(TS)] work inside ts_rs, which it currently does not).
(#134)

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.

Comment on lines 10 to 12
impl TS for serde_json::Value {
type WithoutGenerics = Self;

Copy link
Collaborator Author

@NyxCode NyxCode Mar 19, 2024

Choose a reason for hiding this comment

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

The impl is pretty much exactly like the one generated by

#[derive(TS)]
#[ts(untagged, export_to = "serde_json/")]
enum JsonValue {
    String(String),
    Number(i32),
    Array(Vec<TsJsonValue>),
    Object(HashMap<String, JsonValue>),
}

with the exception that it uses { [key: string]: JsonValue } instead of Record<string, JsonValue>.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 19, 2024

I just realized that what would work is

#[derive(TS)]
#[ts(untagged, export_to = "serde_json/")]
enum TsJsonValue {
    String(String),
    Number(i32),
    Array(Vec<TsJsonValue>),
    Object(#[ts(type = "{ [key: string]: TsJsonValue }")] ()),
}

since the Array variant already creates a dependency on TsJsonValue.

We can't do that in ts_rs right now, however - see the comment on the manual implementation.

@NyxCode NyxCode mentioned this pull request Mar 19, 2024
3 tasks
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo left a comment

Choose a reason for hiding this comment

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

I really like these changes, if you want I can push some commits to #274 so we can merge it faster as well as create a PR for #134

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

I've merged #277 and #274 into this branch to see how that'd look like.
However, now imports don't always work, and I'm not sure yet why.

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

Nevermind - impl_shadow did not generate fn output_path(). Never needed it before, because all types we shadowed weren't exportable.

@escritorio-gustavo
Copy link
Contributor

Awesome! Can this PR be merged or is there anything else you'd still like to do?

#[derive(TS)]
#[ts(
crate = "crate",
rename = "Value",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Last thing I'm unsure about here: Is Value to vague? We currently don't/can't deal with name collisions, so maybe JsonValue might be a better name.

Copy link
Contributor

Choose a reason for hiding this comment

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

JsonValue would be a better name, but given the use of #[ts(export_to = "serde_json/")] I think a name collision is really unlikely

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, lemme quickly rename it. The name collision i'm more worried about is

import type { Value } from "../serde_json/Value";
import type { Value } from "../somewhere/else/Value";

Copy link
Contributor

@escritorio-gustavo escritorio-gustavo Mar 20, 2024

Choose a reason for hiding this comment

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

Oh, I see. I thought you were worried about the file getting overwritten

@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

Alright, let's get this merged!
I'm super happy with how #277 and #274 came together.
And we're finally getting some use out of the effort we put in to export paths and recusrive exports.

@escritorio-gustavo escritorio-gustavo merged commit 8be94ea into main Mar 20, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the serde-json-impl branch March 20, 2024 17:38
@NyxCode
Copy link
Collaborator Author

NyxCode commented Mar 20, 2024

I guess this demonstrates that libraries can really just slap #[derive(TS)] on their types. Maybe, in the future, we won't have to add support for every small crate, but they add support for us :D

escritorio-gustavo added a commit that referenced this pull request Mar 21, 2024
@escritorio-gustavo
Copy link
Contributor

I guess this demonstrates that libraries can really just slap #[derive(TS)] on their types. Maybe, in the future, we won't have to add support for every small crate, but they add support for us :D

That will be awesome to see!

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.

3 participants