-
Notifications
You must be signed in to change notification settings - Fork 461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: change guards for BigInt to NAPI_VERSION > 5 #694
src: change guards for BigInt to NAPI_VERSION > 5 #694
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we need to make changes on test/binding.cc
and in the specific in the following lines of code:
- https://github.com/nodejs/node-addon-api/blob/master/test/binding.cc#L20
- https://github.com/nodejs/node-addon-api/blob/master/test/binding.cc#L76
As reported in the comments we should use NAPI_VERSION
instead of NODE_MAJOR_VERISION
.
@NickNaso I have added the changes you requested to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed in the previous review. Maybe we should consider to remove the comment placed before the guards. Apart this it's good for me.
@@ -17,7 +17,7 @@ Object InitBasicTypesValue(Env env); | |||
// Currently experimental guard with NODE_MAJOR_VERISION in which it was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this comment is not necessary anymore.
@@ -73,7 +73,7 @@ Object Init(Env env, Object exports) { | |||
// Currently experimental guard with NODE_MAJOR_VERISION in which it was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this comment is not necessary anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @NickNaso comments are addressed
dce9dd0
to
507b343
Compare
We need to cast the `nullptr` to the templated type of the `AsyncProgressQueueWorker`. Fixes: nodejs#695 PR-URL: nodejs#696 Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Since we have made the decision that we shall include `BigInt` into N-API 6, we can change the guards for the `BigInt` and `BigInt`-based typed array wrappers accordingly.
507b343
to
44e7688
Compare
Since we have made the decision that we shall include
BigInt
intoN-API 6, we can change the guards for the
BigInt
wrappersaccordingly.