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

BigInt creation and extraction via JSI #775

Closed
RedBeard0531 opened this issue Jul 12, 2022 · 3 comments
Closed

BigInt creation and extraction via JSI #775

RedBeard0531 opened this issue Jul 12, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@RedBeard0531
Copy link
Contributor

Problem

Now that #510 is completed, there is support for BigInt in hermes and JSI. Unfortunately the JSI support is minimal at best. In particular there is no clear way to create or extract a BigInt value from C++ code. It may be possible to go via strings, but that is much more complicated and presumably less efficient than an int64/uint64_t constructor and accessor. Our use case is losslessly working with a C++ library that uses int64_t and we would like that to be as efficient as possible.

Solution

The NAPI BigInt API (docs) is a reasonable starting point here, obviously modified to fit JSI conventions. Key things are that there is a clear and hopefully efficient way to go to and from u/int64_t. It is also nice that there is a combined operation for extracting and checking that the value fits within the destination type. Of course if that isn't provided, it is possible to roll our own by making static(ish) BigInts at the extreme values and doing the comparisons on every extraction, but I assume there is a more efficient implementation possible.

From a quick glance, it looks like the internal BigIntPrimitive type already has the required functions for creating a BigInt, so that it should mostly just be a matter of plumbing to expose them in JSI. I don't immediately see an extraction API to u/int64_t, but I don't think it would be too complicated to either build one on top of getRawData(), or make a new API working directly from the internal representation for efficiency.

@RedBeard0531 RedBeard0531 added the enhancement New feature or request label Jul 12, 2022
@jpporto
Copy link
Contributor

jpporto commented Jul 18, 2022

Hi,

Thank you for your feedback! We left jsi's BigInt interface lacking by design -- we weren't sure what kind of support the community would want, and we wanted to avoid adding APIs that no one used.

Your asks are fairly reasonable, and the team is already working on this. Hopefully you'll see something shortly.

@RedBeard0531
Copy link
Contributor Author

I was about to ask for an update as we are almost ready to start on our JSI port now that our NAPI binding is wrapping up. Then I noticed, that it looks like this is already done via e8cc573. Is there more work to be done before those are ready to be used, or can this ticket be closed?

@jpporto
Copy link
Contributor

jpporto commented Nov 1, 2022

Hi,

They should be ready to use, so give them a try. And let us know if you have any issues with them.

John Paul

@jpporto jpporto closed this as completed Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants