classify js bigint type correctly

[cometkim]

currently, Js.Types.classify to external bigint value matches JSSymbol('a)

This is also related to #5350
I guess bigint_val will be replaced with the bigint binding

[bobzhang]

@cometkim hi, would you mind update the PR with smaller diffs

[cometkim]

what does that mean "smaller diffs"? I only changed the implementation of Js.Types.classify and relevant interfaces and tests.

[bobzhang]

@cometkim sorry, I was on leave for several months. I mean not check in diffs of generated code

[cannorin]

ts2ocaml would greatly benefit from having bigint supported in rescript out of the box.

I'm willing to help this if you need a hand.

[cometkim]

@bobzhang I just cleaned the PR.

I'm also wondering if there are any latest plans for bigint first-class support.

[cristianoc]

@bobzhang I just cleaned the PR.

I'm also wondering if there are any latest plans for bigint first-class support.

would you rebase on current master -- looks like we can move on with this

[cristianoc]

Small suggestions on the order of checks and final default.
Probably does not matter much but might be slightly more future-proof.

[cristianoc]

This looks great. Seems ready to merge.
Just a quick question: are the existing tests OK, do they test enough functionality of typeof?

[cristianoc]

The context is this changes so rarely that I guess it might be unlikely that more tests would catch anything in future. If so, nothing else to do.

[cristianoc]

Also would you update the changelog.

[cristianoc]

(CI fails because some generated files were not checked in after compiling).

[cometkim]

Oh, thanks for updating the changelog :)

[cometkim]

are the existing tests OK, do they test enough functionality of typeof?

Tests are OK, and I think it's enough here.
I couldn't write a decoder without BigInt support, but now I can explicitly output errors at least.

[DZakh]

If we're going to have support for BigInt in the future, maybe it's better to create Js.BigInt module with only type t?
So we won't have breaking changes later.

[DZakh]

If we're going to have support for BigInt in the future, maybe it's better to create Js.BigInt module with only type t? So we won't have breaking changes later.

@cristianoc I don't know how notifications work in GH, so I decided to mention you just in case 😅

I can prepare a PR with it using #5350 as a reference, or even revive it.

[cristianoc]

Sure go ahead.

[DZakh]

I've created a PR in which added the BigInt module #5531. I think it should be good enough as the first iteration.