You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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?
@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.
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.
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.
@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 ?
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.
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.
@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.
Thank you very much @anisha-rohra. I confirm the check works for the Date:
Napi::Valuevalue;
Napi::FunctiondateFunc=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?
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.
👋 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.
@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.
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.
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.
@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)
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.
Activity
jasongin commentedon Jun 12, 2017
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?
addaleax commentedon Jun 12, 2017
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
mhdawson commentedon Jun 12, 2017
@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.
mhdawson commentedon Jun 12, 2017
Can you also paste a snippet of an example of val.IsInt32() being used so we can see the pattern being applied.
sampsongao commentedon Jun 13, 2017
So the current list of type check API's are:
And node-sqlite3 uses IsInt32 in the following method
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.
jasongin commentedon Jun 13, 2017
JSRT doesn't offer any good way to:
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.
mhdawson commentedon Jun 20, 2017
You don't show IsInt32 in the the full list, is that the only one omitted ?
mhdawson commentedon Jun 20, 2017
@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 ?
jasongin commentedon Jun 20, 2017
@mhdawson You can see the available conversion APIs in the link above:
JsConvertValueToBooleanJsConvertValueToNumberJsConvertValueToObjectJsConvertValueToStringAlso 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.
mhdawson commentedon Jun 20, 2017
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.
mhdawson commentedon Jun 20, 2017
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.
jasongin commentedon Jun 20, 2017
If it's a number that is not an int32,
JsNumberToIntconverts it to int32: https://github.com/Microsoft/ChakraCore/blob/d3b6d103bbfa6207d18b74cbf77ca9a48455b8e6/lib/Jsrt/Jsrt.cpp#L1127bsrdjan commentedon Jun 6, 2018
@mhdawson, according to V8 documentation, IsInt32, IsUint32 and IsDate methods are supported by V8, also other IsXXX methods.
mhdawson commentedon Jun 6, 2018
@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.
mhdawson commentedon Jun 18, 2018
@anisha-rohra can you show the code that you are using for isDate?
anisha-rohra commentedon Jun 21, 2018
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.
bsrdjan commentedon Jun 21, 2018
Thank you very much @anisha-rohra. I confirm the check works for the Date:
Once the check is done, which kind of conversion to one of sqlite3 date formats would you consider for sqlite3:
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?mhdawson commentedon Jun 21, 2018
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.
springmeyer commentedon Jul 2, 2018
👋 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.
devsnek commentedon Jul 2, 2018
@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.
mhdawson commentedon Jul 4, 2018
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.
springmeyer commentedon Jul 7, 2018
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.
mhdawson commentedon Jul 9, 2018
@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)
mhdawson commentedon Jul 9, 2018
And I'm hoping we should have that within a week or so.
bsrdjan commentedon Jul 13, 2018
Following the check logic in PR:
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.
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?
github-actions commentedon Dec 19, 2020
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.
mhdawson commentedon Dec 21, 2020
I think the issues discussed have been resolved, closing. Please let me know if you think that was not the right thing to do.