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

Are usize and i64 not supported? #24

Open
affanshahid opened this issue Nov 27, 2022 · 12 comments · May be fixed by #140
Open

Are usize and i64 not supported? #24

affanshahid opened this issue Nov 27, 2022 · 12 comments · May be fixed by #140
Labels
bug Something isn't working feature-request A request for a new feature

Comments

@affanshahid
Copy link

Ran typeshare on my project and got the following errors:

thread '<unnamed>' panicked at 'failed to parse a rust type: ["usize"]', /Users/affan/.cargo/registry/src/github.com-1ecc6299db9ec823/typeshare-cli-1.0.0/src/main.rs:209:17
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at 'failed to parse a rust type: ["i64"]', /Users/affan/.cargo/registry/src/github.com-1ecc6299db9ec823/typeshare-cli-1.0.0/src/main.rs:209:17

i64 can be represented using a JS BigInt.

@jeremychone
Copy link

as with ts-rs would be nice to have a #[typeshare(type = "number")] property attribute.

@snowsignal snowsignal self-assigned this Dec 1, 2022
@snowsignal
Copy link
Contributor

Thanks for opening this issue. I'll look into whether this is possible.

@BenJeau
Copy link

BenJeau commented Dec 31, 2022

Getting a similar error with u64 and isize, those four types seems to be "disabled" here

"u64" | "i64" | "usize" | "isize" => {
return Err(RustTypeParseError::UnsupportedType(vec![id]))
}

But the types exist in the SpecialRustType and looking in the four supported languages, they all have mappings for those four types.

Locally I've changed those three lines in rust_types.rs to parse the types and I'm not getting any errors, but I'm guessing that there must be a reason that those lines/errors exist? It works for Go, Swift, TypeScript, and Kotlin.

Changes

In rust_types.rs:

-                    "u64" | "i64" | "usize" | "isize" => {
-                        return Err(RustTypeParseError::UnsupportedType(vec![id]))
-                    }
+                    "u64" => RustType::Special(SpecialRustType::U64),
+                    "usize" => RustType::Special(SpecialRustType::USize),
+                    "i64" => RustType::Special(SpecialRustType::I64),
+                    "isize" => RustType::Special(SpecialRustType::ISize),

and in typescript.rs:

            SpecialRustType::U64
            | SpecialRustType::I64
            | SpecialRustType::ISize
            | SpecialRustType::USize => {
-                panic!("64 bit types not allowed in Typeshare")
+                Ok("number".into())
            }

Edit: I'm guessing the reason it is disabled because this would not be a safe FFI type for TypeScript/JavaScript, since it may not support 64 bit types depending on the platform. Seems like we would just need to convert it to bigint https://www.typescriptlang.org/docs/handbook/typescript-in-5-minutes-func.html?#built-in-types, where the previous block of code would be:

            SpecialRustType::U64
            | SpecialRustType::I64
            | SpecialRustType::ISize
            | SpecialRustType::USize => {
-                panic!("64 bit types not allowed in Typeshare")
+                Ok("bigint".into())
            }

@Lucretiel
Copy link
Contributor

I think part of the problem is that JSON is usually going to be the medium of exchange for typeshared data, and so when you're loading into javascript / typescript you'd need to not use JSON.parse, since it doesn't care what type you intended and will just load into a number. A lot of the complexity around typeshare enums is similarly related to conforming to the default behavior of various languages when dealing with their equivalent of enums.

@BenJeau
Copy link

BenJeau commented Jan 5, 2023

I understand that most people would use JSON (as it is one of the most popular transport medium), but it is not the only transport medium in which typeshare types can be used, e.g. FFIs

If users of typeshare decide to use it with 64bit types, maybe the library should just give a warning and maybe suggest an alternative library instead of the builtin JSON.parse/JSON.stringify, such as https://github.com/blitz-js/superjson, for the TypeScript/JavaScript side? It seems like this feature is blocked on the fact that JavaScript does not play well by default with 64bit values in JSON, whilst other languages are fine.

@jeremychone
Copy link

Since typeshare is a developer tool, I would recommend picking a default (failing or bigint are both reasonable) and letting the developer override it by property.

In many circumstances, 2^53-1 or 2^64-1 can be considered equivalent (v.s. the complexity of introducing bigint in all JS code base). Obviously, this should not be the default, but not providing this escape hatch might add unnecessary code complexity downstream.

@snowsignal snowsignal added bug Something isn't working feature-request A request for a new feature labels Feb 12, 2023
@snowsignal snowsignal removed their assignment Jul 13, 2023
@mattyg
Copy link

mattyg commented Feb 18, 2024

I just ran into this -- I also need to serialize u64 types. Seems like an oversight to always panic when encountering these types as there are types in the destination languages to support large numbers. At least let the developer set the type themselves.

@AlexanderProd
Copy link

@mattyg A workaround for this is to use #[typeshare(serialized_as = "number")].

@Uzaaft
Copy link

Uzaaft commented Aug 26, 2024

@mattyg A workaround for this is to use #[typeshare(serialized_as = "number")].

This workaround doesn't work. Tested stable and the beta branch.

@LuminaSapphira
Copy link
Collaborator

I think it would make sense for us to support these and emit a suppressable warning if they are used. @Lucretiel

@tomjw64
Copy link

tomjw64 commented Sep 28, 2024

@LuminaSapphira I made a PR for supporting this about a year back. What do you think of that approach (i.e. allow only if the output type is explicitly specified?)

@tomjw64
Copy link

tomjw64 commented Sep 28, 2024

I resolved the existing merge conflicts in the above PR. I would still really appreciate a review + workflow approval from a maintainer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature-request A request for a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants