-
Notifications
You must be signed in to change notification settings - Fork 465
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
Create a doc for migration #118
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 would add a paragraph explaining how different is ObjectWrap between NAN and node-addon-api. In NAN, JavaScript methods are defined via static C++ methods. In node-addon-api, they are defined as C++ instance methods. It is a significant difference.
@sampsongao are you going to add the paragraph requested by @mcollina ? I'm waiting on that to do a thorough review. |
Will do that next week. Currently busy with some other stuff. |
1c5aa3d
to
a2a7962
Compare
@sampsongao ping ? |
@sampsongao, probably also needs to be sync'd with some of the other doc work being done. |
@sampsongao are you still going to be able to update ? |
I don't have anything to update on my side. |
@mcollina if your comments have been addressed can you review/approve? |
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
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
PR-URL: #118 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Landed as 7f3ca03 |
PR-URL: nodejs/node-addon-api#118 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs/node-addon-api#118 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs/node-addon-api#118 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs/node-addon-api#118 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Feel free to comment about things that are not covered here.