Skip to content
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

Do we need wrapper_? #140

Open
gabrielschulhof opened this issue Jun 10, 2020 · 3 comments
Open

Do we need wrapper_? #140

gabrielschulhof opened this issue Jun 10, 2020 · 3 comments

Comments

@gabrielschulhof
Copy link
Contributor

In our ObjectWrap N-API examples we store the napi_ref returned from napi_wrap() in the native object instance although we never use it. To napi_delete_reference() in the destructor we need to also store the napi_env on the native instance – a practice we discourage.

Do we do this for illustration purposes? Can we remove this, since wrapper_ is not being used anywhere?

@gabrielschulhof
Copy link
Contributor Author

This is the extent to which we use wrapper_ and env_:

MyObject::MyObject(double value)
: value_(value), env_(nullptr), wrapper_(nullptr) {}
MyObject::~MyObject() {
napi_delete_reference(env_, wrapper_);
}

and

obj->env_ = env;
status = napi_wrap(env,
jsthis,
reinterpret_cast<void*>(obj),
MyObject::Destructor,
nullptr, // finalize_hint
&obj->wrapper_);

@gabrielschulhof
Copy link
Contributor Author

Compare with the Nan implementation. The use of the persistent reference may stem from there. If so, it may be a good time to diverge, especially since we document that we discourage storing the napi_env.

@gabrielschulhof
Copy link
Contributor Author

In fact, none of the Nan examples store a persistent reference to the JS instance. We should remove it.

gabrielschulhof pushed a commit to gabrielschulhof/node-addon-examples that referenced this issue Aug 5, 2020
Signed-off-by: Gabriel Schulhof <[email protected]>
Fixes: nodejs#140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant