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

Optional schema object as parse arguments to enforce BigInt vs Number #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

haoadoreorange
Copy link
Contributor

@haoadoreorange haoadoreorange commented Nov 27, 2021

This might solve the limitation o !== JSONbig.parse(JSONbig.stringify(o))

The limitation is there because JS treat BigInt and Number as 2 separate types which cannot be cooerced. The parser decides the appropriate type based on the size of the number in JSON string. Which introduces 2 problems:
- As stated above, JSONbig.parse(JSONbig.stringify(123n)) return 123 because the number is small enough
- The type of one field is not consistent, for example one API can return a response in which a field can sometimes be BigInt and other times be Number

There's the option to parse all Number as BigInt but imho this isn't much desirable. Libraries solve problem (2) by iterating the parsed result and enforce the type as you can see here. That PR has an interesting approach which this one is inspired from.

Here's the proposed solution. The parser can take a schema-like object which allow to say whether to use BigInt or Number for specific fields and parse it accordingly. This is a first prototype to show what it might look like.

If this is something viable there's a lot to improve.

Signed-off-by: haoadoresorange <github@comtam.dev>
@haoadoreorange haoadoreorange changed the title Take schema obn parse function to enforce bigint Optional schema object as parse arguments to enforce big Optional schema object as parse arguments to enforce BigInt vs Number Nov 27, 2021
@haoadoreorange
Copy link
Contributor Author

haoadoreorange commented Nov 29, 2021

Since I'm needing this, I rewrote the lib in TS with support for this PR here: https://github.com/haoadoresorange/when-json-met-bigint

@sidorares
Copy link
Owner

hi @haoadoresorange thanks for the PR (and sorry for long silence). Could you add a unit test to cover functionality you are adding?

@haoadoreorange
Copy link
Contributor Author

hi @haoadoresorange thanks for the PR (and sorry for long silence). Could you add a unit test to cover functionality you are adding?

Hi,

Unfortunately at the moment I have no interest in continuing this PR ): as I said I forked the project and implemented this PR here https://github.com/haoadoresorange/when-json-met-bigint

Also I fixed several bugs to align with the newest spec behaviour (: you can give it a look if you like.

@haoadoreorange
Copy link
Contributor Author

haoadoreorange commented Mar 25, 2022

Hi @sidorares, me again 😄

I talked to @MatthewKhouzam today (I believe he talked to you a couple days ago). So apparently merging my forked back to the main repo will benefit greatly everyone, and I'd be gladly to do so !

However the fork as of now is very much diversed from this repo, the original core is similar though. So now I'm not sure how to move forward with the merge and I need your help to make it work (: Could I reach you so we can talk about this a little bit more in detail ?

EDIT: typo

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.

2 participants