Update for support node 12

[fanatid]

Blocked by nodejs/nan#849

Currently only changed to use Maybe version.

[fanatid]

@rvagg can you check this? secp256k1-node which I maintain need bignum with node12 support

[rvagg]

yeah, doesn't compile for me

This whole addon needs to be converted to node-addon-api but I neither have the time or the expertise to pull that off with any reliability.

[fanatid]

With this PR bignum building not work for you on node12?

[rvagg]

no, tests segfault with an unchecked Maybe failure leading to an abort

[devsnek]

what if this used js bigints in >= v12, and the native addon in others? that would fix the build issues and probably give perf boosts as well.

[sam-github]

% git diff
diff --git a/bignum.cc b/bignum.cc
index 9b47ec2..81f9a3f 100644
--- a/bignum.cc
+++ b/bignum.cc
@@ -412,11 +412,12 @@ NAN_METHOD(BigNum::New)
     for (int i = 0; i < len; i++) {
       newArgs[i] = info[i];
     }
-    Local<Value> obj = Nan::New<Function>(js_conditioner)->
-      Call(currentContext, ctx, info.Length(), newArgs).ToLocalChecked();
+    Local<Value> obj;
+    const int ok = Nan::New<Function>(js_conditioner)->
+      Call(currentContext, ctx, info.Length(), newArgs).ToLocal(&obj);
     delete[] newArgs;

-    if (!*obj) {
+    if (!ok) {
       Nan::ThrowError("Invalid type passed to bignum constructor");
       return;
     }

^--- With above patch on top of this PR the tests pass.

[fanatid]

@sam-github thank you! Updated.

[sam-github]

devsnek commented 5 hours from now

First time for me seeing time travellers on github!

This is just a drive-through comment, I don't have any dependencies on bigint, but I think @devsnek has a reasonable suggestion. Also, while my patch will unblock this PR if its hurting downstream users, its not in general a good idea to use ToLocalChecked() everywhere, aborting is a pretty harsh penalty for invalid args passed to a js API.

[sam-github]

@rvagg PTAL

[rvagg]

Sounds like we have a solution. I don't think I'll be in a position to even test this in the next few days so bear with me.

[qix]

@rvagg / @justmoon - This change works for me. Any chance we can get this in an official release?

Thanks!

[fanatid]

While this can be temporary solution, now I agree with @rvagg that addon needs to be converted to node-addon-api. I tried it on previous week and this is nice simple thing, but I'm not sure that I have time for big PR with update everything :(

[rvagg]

pulled and squashed in #124 plus a few additions to get v13 support

[fanatid]

Thanks!