-
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
Objectwrap and ClassPropertyDescriptor documentation #321
Conversation
doc/object_wrap.md
Outdated
InstanceMethod("SetValue", &Example::SetValue) | ||
}); | ||
|
||
constructor = Napi::Persistent(func); |
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.
Some comments here that talk about how the persistent is used and why suppress destruct is needed would be good.
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 it's reasonable give more explanation about this.
doc/object_wrap.md
Outdated
}; | ||
|
||
Napi::Object Example::Init(Napi::Env env, Napi::Object exports) { | ||
Napi::HandleScope scope(env); |
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.
Is the handle scope needed here? I'm not sure but worth checking
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.
No it's not necessary I will remove it. (I tested the example without handle scope).
Ran out of time today, will try to add more comments tomorrow. |
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, added a few changes. @NickNaso I'll go ahead and land, but it probably makes sense for you to review the final version to make sure you like the changes I made.
PR-URL: #321 Reviewed-By: Michael Dawson <[email protected]>
Landed as 100d0a7 |
PR-URL: nodejs/node-addon-api#321 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#321 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#321 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs/node-addon-api#321 Reviewed-By: Michael Dawson <[email protected]>
Hi everyone,
I just start on writing documentation for
Napi::ObjectWrap
please review and suggest me some changes and possible improvements.