Node-addon-api: Check if Napi::Value is integer number?

[bsrdjan]

How to check if Napi::Value is integer number?

v8::Value offers IsNumber, IsInt32 and IsUint32 methods and Napi::Value has only IsNumber. Would it make sense to add integer checks to Napi::Value?

[mhdawson]

How important/useful is it? We don't necessarily need everything which is in V8 to be exposed through Napi. As an example, if other JavaScript engines don't provide it we don't want to have to be building our own implementation. However, on the other hand if its important enough we could consider it.

[bsrdjan]

I am experimenting with node-rfc migration to Napi and my use-case is type check of JavaScript integer value, before passing to native C/C++ library:

https://github.com/SAP/node-rfc/blob/master/src/rfcio.cc#L219

The native lib cannot check if supplied value is really integer and if the float for example is sent instead, the decimals are silently ignored.

Catching such errors makes apps' troubleshooting easier and from that perspective the feature looks important to me. If not done in addon, the check is required on application level, or another addon wrapper, both less convenient.

I could imagine such type check helpers interesting in general because addons are bridges between weak typed JavaScript and strong typed C/C++ and good validation helps.

In my case also the performance of IsString and IsNumber is very important because methods can be invoked on bigger datasets. Are they fast enough for bigger datasets processing?

[mhdawson]

@digitalinfinity does ChakraCore provide similar check methods?
@gabrielschulhof how about JerryScript?

Just looking for additional data.

[mhdawson]

Also @digitalinfinity if ChakraCore does provide the check does it use some information maintained on the Value to do the check efficiently?

[digitalinfinity]

ChakraCore doesn't today AFAIK- JSRT has generally aligned with the ECMAScript spec. I imagine we could implement the API but we've generally avoided adding APIs that are easily implementable by the embedder. We could implement this in N-API without implementing a new ChakraCore API but the ChakraCore API would be more efficient since we do support natively tagged integers/numbers.

cc @MSLaguana

[bsrdjan]

Found related issue #57 and in my case the IsInt32 and optionally IsDate would be highly appreciated.

These two missing methods are the only reason for keeping the napi branch of my project still experimental, beside the nan based master.

[mhdawson]

@bsrdjan thanks for the update, I added a comment on #57 and will see if we can get an update from @anisha-rohra in this weeks meeting.

[DaAitch]

you could do

    if (info[0].IsNumber()) {
      Napi::Value num = info[0].ToNumber();
      double numInt = (double)num.Int64Value();
      double numDbl = num.DoubleValue();

      if (abs(numInt - numDbl) < EPSILON) {
        // maybe was an integer, hard to find the EPSILON,
        // because it depends on the number range
      }
    }

But I think this is not how we should check that. What is the difference between 1.0 and 1? Would it be a problem if internal representation of the number is 1.0, that you will not use 1.

[bsrdjan]

@DaAitch, 1.0 and 1 might work well, I didn't test. The bigger issue is when 1.234 for example is sent, decimals are silently ignored by underlying c++ binary lib and 1.234 float becomes a regular integer 1.

The gap was discovered when the frontend business app was sending 1.234, via nodejs/napi middleware to ABAP backend. App put 1.234 float into wrong JSON structure field, in which ABAP backend expects an integer. As a result, instead of 1.234 float, the int 1 was stored in another field, without complains.

The gap was the reason for adding the check in addon, using handy Nan method:

https://github.com/SAP/node-rfc/blob/master/src/rfcio.cc#L219

Seeing the check existing in v8, I expected having in N-API, would really help here:

https://github.com/SAP/node-rfc/blob/napi/src/rfcio.cc#L240

The above logic like could work in user interactive apps I think. My concerns are batch processing and higher data volumes, when hundreds thousands of rows, each with hundreds of fields are processed. In that case, one simple IsInt() would be worth of gold :)

[DaAitch]

@bsrdjan I understand. If you're brave enough this may help you (didn't test snippet) :)

bool IsInt(Napi::Env& env, Napi::Value& num) {
  return env.Global()
    .Get("Number").As<Napi::Object>()
    .Get("IsInteger").As<Napi::Function>()
    .Call({num}).ToBoolean().Value();
}

so you can also get rid of IsNumber:

Number.isInteger(3); // true
Number.isInteger(3.4); // false
Number.isInteger('hi'); // false
Number.isInteger(Symbol(4)) // false

[bsrdjan]

thanks @DaAitch for the hint. I could not make it work, receiving 'Invalid argument' error but suppose the solution this way is possible. Looking for stable, simple and fast N-API interface for this check, I wait for the #57 outcome.

[bsrdjan]

no IsInteger() function:

isIntegerFunction = env.Global().Get("Number").As<Napi::Object>().Get("IsInteger").As<Napi::Function>()

isIntegerFunction.IsUndefined() -> true

[mhdawson]

Still refining the implementation, but this PR shows how to do IsData and IsRegex and Is32int type checks: https://github.com/mhdawson/node-sqlite3/pull/3

[DuBistKomisch]

+1

Migrating some code now which uses IsUint32, I suppose with IsNumber + Uint32Value you still end up with an integer anyway, but would be nice to enforce more checking.

[asfernandes]

Edit: the problem I was having was another thing.

I have a situation that JS code is passing 0 as argument, I received it in C++ and info[2].IsNumber() == true but when I call info[2].Int32Value() it throws Invalid argument error.

Why 0 is not an integer?

[NickNaso]

Hi @asfernandes,
could you try this info[2].As<Napi::Number>().Int32Value() ? If it does not work I need to investigate more deeply what happen in this specific case.

[addaleax]

Btw, there’s the related threads: https://github.com/nodejs/node/issues/26323 and, to lesser degree, https://github.com/nodejs/node-addon-examples/issues/91.

tl;dr: If you want to check whether a number is an integer, you can use DoubleValue() and check for integer-ness by casting back and forth once: (double)((int64_t)(double_value)) == double_value.

[asfernandes]

@NickNaso it seems my problem was another but my logs wasn't working properly. My usage of ToNumber() seems to work as I was expecting.

[jorangreef]

@bsrdjan We had the same requirements as you. This is what we ended up using to get an argument as an integer, rejecting NaN, +/-Infinity and floats:

It's not engine dependent, but it is tricky, you have to check for undefined behavior before casting the double to an integer. I'm not sure even this is right and would appreciate feedback. Perhaps it makes sense to have this in N-API to get more eyes on it.

#include <math.h>

static int arg_int(napi_env env, napi_value value, int* integer) {
  double temp = 0;
  if (
    // We get the value as a double so we can check for NaN, Infinity and float:
    // https://github.com/nodejs/node/issues/26323
    napi_get_value_double(env, value, &temp) != napi_ok ||
    // Reject NaN:
    isnan(temp) ||
    // Reject Infinity and avoid undefined behavior when casting double to int:
    // https://groups.google.com/forum/#!topic/comp.lang.c/rhPzd4bgKJk
    temp < INT_MIN ||
    temp > INT_MAX ||
    // Reject float by casting double to int:
    (double) ((int) temp) != temp
  ) {
    napi_throw_error(env, NULL, "argument must be an integer");
    return 0;
  }
  *integer = (int) temp;
  return 1;
}

If need be, one could replace the ints above with int64_t.

[a7ul]

@bsrdjan I understand. If you're brave enough this may help you (didn't test snippet) :)

bool IsInt(Napi::Env& env, Napi::Value& num) {
  return env.Global()
    .Get("Number").As<Napi::Object>()
    .Get("IsInteger").As<Napi::Function>()
    .Call({num}).ToBoolean().Value();
}

so you can also get rid of IsNumber:

Number.isInteger(3); // true
Number.isInteger(3.4); // false
Number.isInteger('hi'); // false
Number.isInteger(Symbol(4)) // false

I solved it using this as a reference. But instead of IsInteger it should be isInteger.

bool isNapiValueInt(Napi::Env& env, Napi::Value& num) {
  return env.Global()
      .Get("Number")
      .ToObject()
      .Get("isInteger")
      .As<Napi::Function>()
      .Call({num})
      .ToBoolean()
      .Value();
}

[bsrdjan]

Thanks @master-atul . I am actually fine with @jorangreef approach, using it slightly modified for my requirements. I am closing this issue, waiting for the related #57 resolution.