napi_wrap only allow one wrapper to one object

[sampsongao]

I was updating the node-sass code to the current napi and I found an issue with napi_wrap check. So in napi_wrap, if we see the object is a wrapper, we return napi_invalid_arg. However, in node-sass, we do something like this:

template <class T>
napi_value SassValueWrapper<T>::get_js_object(napi_env env) {
  if (this->js_object == nullptr) {
  napi_value wrapper;
  napi_value ctor = T::get_constructor(env);
  CHECK_NAPI_RESULT(napi_new_instance(env, ctor, 0, nullptr, &wrapper));
  void* wrapped;
  CHECK_NAPI_RESULT(napi_unwrap(env, wrapper, &wrapped));
  delete static_cast<T*>(wrapped);
  CHECK_NAPI_RESULT(napi_wrap(env, wrapper, this, nullptr, nullptr, nullptr)); -----------> check failed
  CHECK_NAPI_RESULT(napi_create_reference(env, wrapper, 1, &this->js_object));
}

[mhdawson]

My first thought is that we should add a

napi_remove_wrap(napi_env env, napi_value wrapper);

This would cover the case where you want to associate new object to a wrapper (which we know is needed by node-sass) and the case where you just want to remove the wrapping from an object.

The other option is simply to remove the existing check and allow the second wrap to just succeed. I'm thinking its better to be sure it was the intention of the developer by having them use napi_remove_wrap.

@nodejs/addon-api what are your thoughts.