Object.getOwnPropertyDescriptor throws on accessor properties of an ObjectWrap class' prototype object

[argv-minus-one]

An error is thrown when trying to get the property descriptor of a Napi::ObjectWrap::InstanceAccessor from the class' prototype object.

Suppose you have a class named DemoClass, which was defined by Napi::ObjectWrap::DefineClass. Suppose it has a property named hello, which was defined by Napi::ObjectWrap::InstanceAccessor. If you then try to Object.getOwnPropertyDescriptor(DemoClass.prototype, "hello"), you get an error:

run.js:2
Object.getOwnPropertyDescriptor(DemoClass.prototype, "hello");
       ^

Error: Invalid argument
    at Function.getOwnPropertyDescriptor (<anonymous>)
    at Object.<anonymous> (run.js:2:8)
    at Module._compile (internal/modules/cjs/loader.js:738:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:749:10)
    at Module.load (internal/modules/cjs/loader.js:630:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:570:12)
    at Function.Module._load (internal/modules/cjs/loader.js:562:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:801:12)
    at internal/main/run_main_module.js:21:11

The reason is that Napi::ObjectWrap::InstanceGetterCallbackWrapper attempts to unwrap this, which doesn't work because DemoClass.prototype is not a wrapper!

But why is the callback even being called? Node checks whether native instance methods are called on a valid instance of the class, and doesn't even call the native function if not. Shouldn't it perform the same checks on native instance accessors?

I have prepared a Gist demonstrating the problem. To use:

1. Download or git clone it.
2. npm install
3. node run.js

[mhdawson]

To me it seems more that maybe InstanceGetterCallbackWrapper should handle being called when Unwrap fails or that it should check in that is has been called on a wrapper.

I don't think we thought anybody would ask for the property or descriptor on the prototype, it's just there so that it is inherited when instances are created for Wrapped objects.

I think we've effectively told the runtime that the InstnaceGEtterCallbackWrapper is a property on the prototype so unless I'm missing something it's reasonable for them to be called.

This is what it would look like if it figured out that it was not called on a wrapper and just returned undefined:

undefined
{
  value: undefined,
  writable: false,
  enumerable: false,
  configurable: false
}

Not sure if that is useful though.

[mhdawson]

With the latest comment this might also have some effect on the behaviour: nodejs/node#27851 (comment)

I'd tried the earlier version but it did not resolve the reported issue.

[mhdawson]

This is the output of the test case provided once the current version of nodejs/node#27851 is applied:

{
  get: [Function (anonymous)],
  set: undefined,
  enumerable: false,
  configurable: false
}

[mhdawson]

Interestingly the change in f nodejs/node#27851 breaks one of our unit tests. My first guess is a test problem as it depends on the order that the keys are reports by "for in"

[mhdawson]

This confirms that depending on ordering is incorrect:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in
A for...in loop iterates over the properties of an object in an arbitrary order (see the delete operator for more on why one cannot depend on the seeming orderliness of iteration, at least in a cross-browser setting).

[GitMurf]

I came across this old Issue but couldn't tell if there was a resolution to this. I am getting the same Invalid argument when trying to access a Getter (from InstanceAccessor) named width (get and set width).

I get the error when trying to access MyObject.prototype.width ... I need to be able to do this for testing purposes. The NaN equivalent to this code allows this and does not throw an error. Are there any options available or workarounds I can implement for Napi?

Here is my example code:

// MyObject.h

class MyObject : public Napi::ObjectWrap<MyObject> {
  private:
    int width = 0;
  public:
    MyObject(const Napi::CallbackInfo& info);
    static void Initialize(Napi::Env& env, Napi::Object& target);
    static Napi::FunctionReference *constructor;
    Napi::Value GetWidth(const Napi::CallbackInfo& info);
    void SetWidth(const Napi::CallbackInfo& info, const Napi::Value& value);
}

// MyObject.cc

Napi::FunctionReference* MyObject::constructor = new Napi::FunctionReference();

void MyObject::Initialize(Napi::Env& env, Napi::Object& target) {
  Napi::HandleScope scope(env);
  Napi::Function ctor = DefineClass(
    env,
    "MyObject",
    {
      InstanceAccessor("width", &MyObject::GetWidth, &MyObject::SetWidth),
    }
  );
  *constructor = Napi::Persistent(ctor);
  target.Set("MyObject", ctor);
}

MyObject::MyObject(const Napi::CallbackInfo& info) : Napi::ObjectWrap<MyObject>(info) {
  Napi::Env env = info.Env();
  if(info[0].IsNumber()) this->width = info[0].As<Napi::Number>().Int32Value();
}

Napi::Value MyObject::GetWidth(const Napi::CallbackInfo& info) {
  return Napi::Number::New(info.Env(), this->width);
}

void MyObject::SetWidth(const Napi::CallbackInfo& info, const Napi::Value& value) {
  if(value.IsNumber()) this->width = value.As<Napi::Number>().Int32Value();
}

Here is example of calling it in JavaScript:

// Calling from JavaScript

MyObject.prototype.width;

Here is the error I am getting:

[GitMurf]

Also please feel free to provide any advice or corrections for my code above if there is anything I am missing or have written incorrectly. I am all ears and open to any and all feedback. Thanks!

[KevinEady]

Hi @GitMurf ,

Instance accessors require an instance in order to get the value. When using MyObject.prototype.width, there is no actual instance of MyObject to get the this->width from. For example:

class MyObject { 
    constructor(width) { 
        this._width = width; // roughly equivalent to: this->width = info[0].As<Napi::Number>().Int32Value();
    }
    get width() {
        return this._width; // roughly equivalent to: return Napi::Number::New(info.Env(), this->width);
    }
}

In order to access the getter, you must have an instance to call the getter from:

Object.getOwnPropertyDescriptor(MyObject.prototype, 'width').get.call(new MyObject(5)) // 5

You can do the same thing above on a Node API ObjectWrap'd instance. Here's a diff from https://github.com/nodejs/node-addon-examples/tree/main/6_object_wrap/node-addon-api:

diff --git a/6_object_wrap/node-addon-api/addon.js b/6_object_wrap/node-addon-api/addon.js
index d7dd6e0..4cec5e6 100644
--- a/6_object_wrap/node-addon-api/addon.js
+++ b/6_object_wrap/node-addon-api/addon.js
@@ -5,9 +5,11 @@ console.log( obj.plusOne() ); // 11
 console.log( obj.plusOne() ); // 12
 console.log( obj.plusOne() ); // 13
 
-console.log( obj.multiply().value() ); // 13
-console.log( obj.multiply(10).value() ); // 130
+console.log( obj.multiply().value ); // 13
+console.log( obj.multiply(10).value ); // 130
 
 var newobj = obj.multiply(-1);
-console.log( newobj.value() ); // -13
+console.log( newobj.value ); // -13
 console.log( obj === newobj ); // false
+
+console.log( Object.getOwnPropertyDescriptor(addon.MyObject.prototype, 'value').get.call(obj) ); // 13
diff --git a/6_object_wrap/node-addon-api/myobject.cc b/6_object_wrap/node-addon-api/myobject.cc
index 262a705..da8a4ee 100644
--- a/6_object_wrap/node-addon-api/myobject.cc
+++ b/6_object_wrap/node-addon-api/myobject.cc
@@ -5,7 +5,7 @@ Napi::Object MyObject::Init(Napi::Env env, Napi::Object exports) {
       DefineClass(env,
                   "MyObject",
                   {InstanceMethod("plusOne", &MyObject::PlusOne),
-                   InstanceMethod("value", &MyObject::GetValue),
+                   InstanceAccessor("value", &MyObject::GetValue, nullptr),
                    InstanceMethod("multiply", &MyObject::Multiply)});
 
   Napi::FunctionReference* constructor = new Napi::FunctionReference();

Hope this helps!