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

Transcribed numbers are all converted from integers into decimals #15

Closed
RonSijm opened this issue Sep 22, 2023 · 6 comments
Closed

Transcribed numbers are all converted from integers into decimals #15

RonSijm opened this issue Sep 22, 2023 · 6 comments

Comments

@RonSijm
Copy link

RonSijm commented Sep 22, 2023

For example:

Source:
https://github.com/LemmyNet/lemmy/blob/24c98a726af9977c6aed4baefaf20143803de059/crates/db_schema/src/newtypes.rs#L54

Lemmy-js converts it into a Number
https://join-lemmy.org/api/interfaces/FollowCommunity.html#community_id

This is technically correct: https://www.typescriptlang.org/docs/handbook/basic-types.html
But it loses precision, and every number becomes a decimal

Everything in the OpenAPI spec becomes a number instead of an integer:
https://github.com/MV-GH/lemmy_openapi_spec/blob/master/lemmy_spec.yaml#L2128C10-L2128C10

Which is incorrect:
https://swagger.io/docs/specification/data-models/data-types/

It looks like Lemmy-js intentionally does this:
https://github.com/LemmyNet/lemmy-js-client/blob/f439a7cedafc003c6513848da7342aacb35eab7b/copy_generated_types_from_lemmy.sh#L22

I think so solve this you'd first have to build the lemmy-js-client without substituting BigInts.

I tried doing all this myself, but I can't get Lemmy to run from source yet

@MV-GH
Copy link
Owner

MV-GH commented Sep 22, 2023

I'll start by making a inquiry on lemmy-js-client. To see if they are open for letting the bigInts stay. ThenI'll look into other approaches. The approach you mentioned is a viable one but it does increase the complexity quite a bit. As I'll still need the original lemmy-js-client for the summaries. And we ll need our own generated types from Lemmy. Another approach is to replace all Numbers with BigInt in the genTypes file.

@MV-GH
Copy link
Owner

MV-GH commented Sep 23, 2023

The approach is for 0.18.4 we bruteforce all numbers into begint by modifying gen_types.ts, then our gen_components.ts should make the ints and in the future(0.19) we modify gen types to selectively turn them into bigints depending on the comments as described in the PR

@MV-GH
Copy link
Owner

MV-GH commented Sep 23, 2023

Oh well its gonna get more complicated because our components generator, still creates number for bigint. Gotta think now how I am gonna solve this

@MV-GH
Copy link
Owner

MV-GH commented Sep 24, 2023

It doesn't support BigInts apparently, I have a made a PR to do so. grantila/core-types-ts#13

@RonSijm
Copy link
Author

RonSijm commented Sep 24, 2023

Haha nice, this is becoming a supply chain "whack-a-mole" where every step of the chain seems to be missing something 🙈

@MV-GH
Copy link
Owner

MV-GH commented Oct 23, 2023

Solved for now by making 0.19 remove all floats. Thus we can use the original assumption that all numbers are float. This will be a problem for another time

@MV-GH MV-GH closed this as completed Oct 23, 2023
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

No branches or pull requests

2 participants