Add BigInt support and more nj-core support for N-API v7. #97
Conversation
nj-sys/src/binding.rs
Outdated
| @@ -1,646 +1,258 @@ | |||
| /* automatically generated by rust-bindgen */ | |||
|
|
|||
| pub const NAPI_VERSION: u32 = 5; | |||
| pub const NAPI_VERSION: u32 = 7; | |||
There was a problem hiding this comment.
I don't wanna preemptively add more scope to this task but I accidentally ended up pushing the wrong bindings earlier because I didn't run make generate in js-sys. If we're open to it, I'd love to move to using bindgen as a build dependency and generate these bindings at build time rather than having them in the source code.
The pros for doing this are that when the next person to come along and update the N-API version, they'll pop in the new headers into the vendor directory, update the version and then just simply run cargo build in the parent directory and deal with whatever bugs exist. It will also make it so you've got a consistent version of bindgen at compile time (I also had this problem).
The cons of this are that build times will be longer and the jump-to-definition feature of rust-analyzer may not work for things in nj-sys :/.
There was a problem hiding this comment.
Initial version of the node-bindgen used build.rs to generate binding. Main issue was build time which was long. Hopefully, current rust compiler performance has improved that that is no longer concern.
Also, there was bug in the bindgen the created inconsistent mapping between versions. I think that could be mitigated by tied to specific version of bindgen
There was a problem hiding this comment.
Initial version of the node-bindgen used build.rs to generate binding. Main issue was build time which was long. Hopefully, current rust compiler performance has improved that that is no longer concern.
Ah. Yeah. That's definitely a con. I'll play around with it and see what the compile time is like.
| let mut words: Vec<u64> = Vec::new(); | ||
| use std::cmp::min; | ||
|
|
||
| // bytes can be non-multiples of 8. |
There was a problem hiding this comment.
I was tempted to use something like byteorder for this because byte mangling like this can be error prone. Also, writing this took way longer that I wanted (and will for the next person). I'm open adding it but as we've seen with rust-bindgen, it'll add more compile time to whatever depends on this project.
* Updated to N-API v7 in nj-sys. * Updated CHANGELOG. * Added detach buffer calls in JsEnv. * Added BigInt for going to-from rust and JS. * Re-export the rust bigint exports.
|
I spent a bunch of time today in my non-computer time occasionally thinking about the need to convert Vec to/from Vec for bigint. Anyway, I finally decided to look into the implementation of the rust BigInt crate used in this PR and well depending on your system it actually does use u64's internally. In rust-num/num-bigint#152, a suggestion to expose the internal data structure BigDigitbut the maintainer has some reservations. Depending on how ambitious I am, I might look into adding aFrom for BigInt` in the bigint crate. I think that'd be the way to gain the best performance and maintainability here. |
|
Can you split these into separate PR? It would be easier to track changes. |
simlay
changed the title
Sure. I've made #99 to pull out the nj-sys changes which is the bulk of this PR. |
This PR will resolve #28 and will also deal with some of the undone parts of #96.
In an unlisted manner this PR should include:
Go get the node.js src, and copy the headers from it forWill be resolved with Added n-api-v7 to nj-sys #99.nj-sys.nj-coretonj-sysfor N-API v6:u64s in rust toBigInts in JS.napi_create_bigint_int64Not applicable.BigInts to rustBigInts usingnapi_get_value_bigint_words andnapi_create_bigint_words.nj-coretonj-sysfor N-API v7:BigIntin new example workspace.CHANGLOG.md