search

concordancejs / concordance

Compare, format, diff and serialize any JavaScript value
ISC License
207 stars 15 forks source link

Support BigInt #43

Closed bitjson closed 6 years ago

bitjson commented 6 years ago

Possibly the beginnings of support for BigInt, feedback appreciated.

I haven't quite wrapped my head around the codebase yet, and still trying to understand some of the test suite. (Really impressed with the coverage tests for themes, by the way).

If you have a different way you'd like to implement this, please feel free to just close this PR.

Closes https://github.com/concordancejs/concordance/issues/42.

novemberborn commented 6 years ago

45 sets up CI to run on Node.js 10. It also removes support for Node.js 4 and early Node.js 6 releases, which also impacts (de)serialization. Consequently we have an opportunity to slot bigint in with the other primitives rather than at the end of the list. AVA users will have to rebuild their snapshots regardless.

bitjson commented 6 years ago

Hey @novemberborn thanks for the quick review and action! I marked the next available codepoint (0x1F) for BigInt and incremented the version. (Following the comments in serialize.js.)

I don't think I'm quite familiar with how things work in the project yet, so please let me know what else I need to change here.

novemberborn commented 6 years ago

@bitjson I think I've addressed all remaining issues. What do you think?

bitjson commented 6 years ago

@novemberborn this looks great! Just tested locally in the project that spawned this issue – works perfectly. :shipit:

bitjson commented 6 years ago

(The working test: https://github.com/bitjson/bitcoin-ts/blob/c030d5f0d623e3014f177a49c1cc69351bbcf19f/src/lib/transaction.spec.ts)