feat: added bigint force long option #742
Conversation
azizghuloum
changed the title
There was a problem hiding this comment.
Nice! This looks great @azizghuloum!
I was assuming we'd have to wait on protobufjs to support bigint natively, but great call that we can just do the adapting on the way in/out.
I had one question about potentially using more mathy to/from conversion methods, from the longfn repo, but definitely optional and happy to merge this as-is. Just let me know, thanks!
| const longToBigint = conditionalOutput( | ||
| "longToBigint", | ||
| code` | ||
| function longToBigint(long: ${Long}) { |
There was a problem hiding this comment.
I was wondering if long.js had a built-in toBigInt and didn't find anything, but I did find a github issue where someone linked to their long.js fork that has a toBigInt:
https://github.com/consento-org/longfn/blob/main/index.mjs#L654
Wdyt about adapting their toBigInt and maybe even fromBigInt?
https://github.com/consento-org/longfn/blob/main/index.mjs#L100
Seems like those would surely be faster than going through toStrings, so seems like a good idea, but also willing to leave it as a follow-up issue if you'd like to ship this as-is.
There was a problem hiding this comment.
Honestly I was just trying to get it to work correctly with zero effort towards optimizing anything. This, and a few other things in the timestamps can definitely be improved, but I think eventually protobuf's Reader and Writer will support bigints and drop Long. Regardless, I think it should be merged now and any improvements be done later.
# [1.137.0](v1.136.1...v1.137.0) (2022-12-29) ### Bug Fixes * Additional fix for structs with useMapType. ([#743](#743)) ([3264b0f](3264b0f)) * Fix codegen for google.protobuf.Struct with useMapType=true ([#740](#740)) ([0647151](0647151)) ### Features * added bigint force long option ([#742](#742)) ([3964e57](3964e57))
|
🎉 This PR is included in version 1.137.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Also added simple tests based on string option.