-
Notifications
You must be signed in to change notification settings - Fork 596
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
Upgrade node-addon-api Object Wrap example for multi-context support #137
Conversation
@@ -5,8 +5,11 @@ | |||
"main": "addon.js", | |||
"private": true, | |||
"gypfile": true, | |||
"engines": { | |||
"node": ">= 14.0.0" | |||
}, |
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 don't think we'd want to add this. If the example needs to be different for multi-context and that is only available in later Node.js versions, then we should add a new example versus changing the old one. Ideally we would have an example that would work across all Node.js LTS versions.
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.
Ok, I changed it and it a new example now. But there is linked bug nodejs/node-addon-api#730
@anfilat There is a newer example of using https://github.com/nodejs/node-addon-examples/tree/master/object-wrap-demo/node-addon-api As far as I can tell, this newer example is context aware. Will you be able to meet your objectives by following the pattern of this newer example? A question for the N-API team. In what cases is a static constructor function required? Or could it be the case that the 6_object_wrap |
@jschlight As I see, there is a similar PR from the N-API team member - #139 |
@anfilat Thank you. I will followup with the N-API team. |
"engines": { | ||
"node": ">= 14.0.0" | ||
}, |
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.
This needs to work with all supported versions of Node.js.
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.
@anfilat using the value for "engiens" as suggested at #139 (comment) should make the CI green, and it should actually test your code on Travis at least, even if it doesn't test it on GitHub actions.
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.
Oh, wait, #139 needs to land to update the .travis.yml removing the non-supported Node.js 8 first 🙂
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.
@gabrielschulhof I think I should close this PR. It's duplicate with #139 now
Seems this issues exists when using cmake-js (6.1.0) instead of gyp , can anyone help? |
It replaced global constructor with new SetInstanceData\GetInstanceData API.