More special types check about node-addon-api HOT 27 CLOSED

@anisha-rohra is now working on the port for node-sqlite3. She is working on implementing the methods using what was suggested in an N-API team meeting and if its something that can be done without additional methods we may start by documenting that.

from node-addon-api.

Following the check logic in PR:

double orig_val = source.As<Napi::Number>().DoubleValue();
double int_val = (double)source.As<Napi::Number>().Int32Value();
if (orig_val == int_val) {

after positive if-statement check, I would have to cast the source again, this time as integer, for passing to SAP/ABAP binding lib:

int32_t int32_val = source.As<Napi::Number>().Int32Value();

To save that one additional cast, the check I am currently using is just a bit different.

double orig_val = source.As<Napi::Number>().DoubleValue();
int32_t int_val = source.As<Napi::Number>().Int32Value();
if ((int32_t)orig_val != orig_val) // or std::trunc(orig_val) == orig_val;

It works with 1, 2 and 4 byte integers and stops if an integer with Number.EPSILON decimal part sent.

Would you consider this check fine as well and eventually more performant?

from node-addon-api.

For the first two, is it possible to use napi_instanceof? You'd have to first obtain the constructor functions for Date and RegExp.

Whether a number value is an int32 vs some other kind of number is an engine-specific optimization that probably doesn't belong in N-API. Is there a way to workaround that in node-sqlite3?

from node-addon-api.

For the first two, is it possible to use napi_instanceof? You'd have to first obtain the constructor functions for Date and RegExp.

That doesn’t do the same thing; for Date objects from other contexts, it won’t work, see e.g. https://github.com/jasnell/proposal-istypes

from node-addon-api.

@sampsongao can you check what the full list of val.IsXXX methods is ? It would be interesting to see how what the full list is as part of this discussion.

from node-addon-api.

Can you also paste a snippet of an example of val.IsInt32() being used so we can see the pattern being applied.

from node-addon-api.

So the current list of type check API's are:

IsEmpty, IsUndefined, IsNull
IsNumber, IsString, IsSymbol, IsObject, IsFunction
IsArray, IsArrayBuffer, IsTypedArray, IsBuffer

And node-sqlite3 uses IsInt32 in the following method

template <class T> Values::Field*
                   Statement::BindParameter(const Local<Value> source, T pos) {
    if (source->IsString() || source->IsRegExp()) {
        Nan::Utf8String val(source);
        return new Values::Text(pos, val.length(), *val);
    }
    else if (source->IsInt32()) {
        return new Values::Integer(pos, Nan::To<int32_t>(source).FromJust());
    }
    else if (source->IsNumber()) {
        return new Values::Float(pos, Nan::To<double>(source).FromJust());
    }
    else if (source->IsBoolean()) {
        return new Values::Integer(pos, Nan::To<bool>(source).FromJust() ? 1 : 0);
    }
    else if (source->IsNull()) {
        return new Values::Null(pos);
    }
    else if (Buffer::HasInstance(source)) {
        Local<Object> buffer = Nan::To<Object>(source).ToLocalChecked();
        return new Values::Blob(pos, Buffer::Length(buffer), Buffer::Data(buffer));
    }
    else if (source->IsDate()) {
        return new Values::Float(pos, Nan::To<double>(source).FromJust());
    }
    else {
        return NULL;
    }
}

I guess removing the IsInt32 case wouldn't affect its functionality, but might fail some of their tests. But IsDate and IsRegExp seems crucial to me in this case.

from node-addon-api.

JSRT doesn't offer any good way to:

• Check if a number (or any value) is an integer
• Check if a value is a Date or RegExp

Maybe we could add that functionality to the JSRT API. And maybe in these cases that is justified. But we should always consider whether additional APIs are actually necessary, so we don't arbitrarily force other JS engines to provide V8-specific functionality.

from node-addon-api.

You don't show IsInt32 in the the full list, is that the only one omitted ?

from node-addon-api.

@jasongin I assume that JSRT does have a way to do the conversion, if so what happens if you pass in a reference that is not the correct type to the conversion functions ? For example you pass in a string and ask it to convert to an Integer or Float ?

from node-addon-api.

@mhdawson You can see the available conversion APIs in the link above:

• JsConvertValueToBoolean
• JsConvertValueToNumber
• JsConvertValueToObject
• JsConvertValueToString

Also it allows getting a number value as either an int32 (JsNumberToInt) or double (JsNumberToDouble).

But those don't get around the limitations I mentioned in my previous comment.

from node-addon-api.

In the case of JsNumberToInt it says it will fail if the argument is not a number but what happens if the value is bigger than what fits into a 32bit value. If it returns an error code then it could be used to implement IsInt32, but if it silently truncates then maybe not.

from node-addon-api.

In terms of things like IsDate() could we do something like stash the prototype for Date somewhere and do a comparison without direct support from the Runtime? I know it may not be optimal.

@sampsongao I think I asked my question incorrectly last time. What I wanted to know was what the full list of IsXXX methods supported by V8.

from node-addon-api.

If it's a number that is not an int32, JsNumberToInt converts it to int32: https://github.com/Microsoft/ChakraCore/blob/d3b6d103bbfa6207d18b74cbf77ca9a48455b8e6/lib/Jsrt/Jsrt.cpp#L1127

from node-addon-api.

@mhdawson, according to V8 documentation, IsInt32, IsUint32 and IsDate methods are supported by V8, also other IsXXX methods.

from node-addon-api.

@anisha-rohra can you show the code that you are using for isDate?

from node-addon-api.

A version of IsDate can be done as follows:

Napi::Function date_func = source.Env().Global().Get("Date").As();
If (source.As().InstanceOf(date_func)) {
// source is a Date
}

I've tested that this also works for IsRegExp, so theoretically it should apply to other JS Objects.

@bsrdjan I hope that this helps - let me know if I can be of further assistance.

from node-addon-api.

Thank you very much @anisha-rohra. I confirm the check works for the Date:

Napi::Value value;
Napi::Function dateFunc = value.Env().Global().Get("Date").As<Napi::Function>();
if (value.As<Napi::Object>().InstanceOf(dateFunc)) {
// it is a date
}

Once the check is done, which kind of conversion to one of sqlite3 date formats would you consider for sqlite3:

• TEXT as ISO8601 strings ("YYYY-MM-DD HH:MM:SS.SSS").
• REAL as Julian day numbers, the number of days since noon in Greenwich on November 24, 4714 B.C. according to the proleptic Gregorian calendar.
• INTEGER as Unix Time, the number of seconds since 1970-01-01 00:00:00 UTC.

In my case, the Date should be transformed into SAP ABAP YYYYMMDD format. I would parse the value.ToString(), assuming the nodejs Date ToString() output format is stable.

Back to my original question, I don't see how to apply this approach to check if expected integer value is possibly sent from nodejs as float? Tried to make the #265 proposal work and getting Error: Invalid argument. Could the function call format be possibly wrong here?

Napi::Function isIntegerFunction = value.Env().Global().Get("Number").As<Napi::Object>().Get("IsInteger").As<Napi::Function>();
bool isInt = isIntegerFunction.Call({value}).ToBoolean().Value();

from node-addon-api.

It sounds like there is a reasonable solution for the methods other than isInt32(). What would be interesting is to look to see if v8 implementation has something which optimizes the check or we can apply the same check in native code. @anisha-rohra could you take a look at then and comment. If you have any questions on what to check just let me know.

from node-addon-api.

👋 node-sqlite3 maintainer here. I'd like to try supporting N-API, and so I'm waiting on this support. Do you anticipate progress soon? If anyone wants to try fixing the type checks that are needed feel free to fork TryGhost/node-sqlite3#1008.

from node-addon-api.

@springmeyer just to clarify, the hesitation here comes from the fact that not all js engines support things like "is date" because the spec for js doesn't specify a production to perform that check. I think you would be best off for the time-being using instanceof checks. number type checks can be done easily with simple comparisons so napi doesn't need to implement that.

from node-addon-api.

Confirming what @devsnek mentioned and to provide some additional info:

@springmeyer @anisha-rohra was already working on those issues in our existing port here:https://github.com/mhdawson/node-sqlite3

and this PR was to provide a solution to 2 of the checks (still needs a bit of clean up) mhdawson/node-sqlite3#3

As for the last one (int32 check) we had looked at the V8 impl and it did not look like the v8 code had any special optimization so that check could be done just as well in the add-on code as well.

@anisha-rohra was working on that as well. Anisha can you give an update on the isdate check so far.

from node-addon-api.

I think you would be best off for the time-being using instanceof checks. number type checks can be done easily with simple comparisons so napi doesn't need to implement that.

Thank you for clarifying. That makes sense. I'm only coming from the node.js side of things and have never used another JS engine so that context was not obvious to me and I appreciate the clarification. That said, I don't have time to code or test these changes myself, so I would ideally need a PR on top of TryGhost/node-sqlite3#1008 (or a freshly created one against master of https://github.com/mapbox/node-sqlite3) to be able to move forward with testing node-sqlite3 support of N-API. I'm making the assumption that node-addon-api developers want to see this happen given the context of TryGhost/node-sqlite3#830, so I'm here just trying to make clear that TryGhost/node-sqlite3#830 is blocked until someone (sounds like it may be @anisha-rohra) proposes a solution.

from node-addon-api.

@springmeyer correct, the end goal of the work @anisha-rohra is currently doing is a PR to @sqlite3 (either a new one or one on top of what you have already)

from node-addon-api.

And I'm hoping we should have that within a week or so.

from node-addon-api.

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

from node-addon-api.

I think the issues discussed have been resolved, closing. Please let me know if you think that was not the right thing to do.

from node-addon-api.