ObjectWrap: enumerable properties seem to be broken #644

Activity

1. legendecas commented on Jan 5, 2020

   

   legendecas

   on Jan 5, 2020

   Member

   More actions

   Instance property accessors are defined on class prototypes, not on class instances.

   Try:

   assert.strictEqual(Object.getOwnPropertyDescriptor(Object.getPrototypeOf(obj), 'testGetter').enumerable, true);
2. gms1 commented on Jan 5, 2020

   

   gms1

   on Jan 5, 2020

   ContributorAuthor

   More actions

   many thanks!
   Is there a way to define enumerable properties on the class instance?
   something like this from within the c-tor?

   info.This().As<Napi::Object>().DefineProperty(...)
   

   didn't get it working and have not found anything in the documentation or in the examples
3. gabrielschulhof commented on Jan 5, 2020

   

   gabrielschulhof

   on Jan 5, 2020

   Contributor

   More actions

   @gms1 you specify per-instance properties as a parameter to the DefineClass() call in your ObjectWrap subclass.
4. gms1 commented on Jan 6, 2020

   

   gms1

   on Jan 6, 2020

   ContributorAuthor

   More actions

   @gabrielschulhof
   thanks, but using DefineClass() with InstanceAccessor(...,napi_enumerable) to specify per instance properties, the specified properties are not enumerable on the instance.
   As @legendecas mentioned, this is because the properties are defined on the prototype and not on the instance
   So having an instance obj of the ObjectWrap subclass, a call to Object.getOwnPropertyNames(obj) or Object.keys(obj) returns an empty array, JSON.stringify(obj) results in "{}", a.s.o.
5. gabrielschulhof commented on Jan 6, 2020

   

   gabrielschulhof

   on Jan 6, 2020

   Contributor

   More actions

   @gms1 confirmed. The following test fails:

   diff --git a/test/objectwrap.cc b/test/objectwrap.cc
   index 0d61171..9c61f9e 100644
   --- a/test/objectwrap.cc
   +++ b/test/objectwrap.cc
   @@ -29,6 +29,22 @@ public:
          finalizeCb_ = Napi::Persistent(info[0].As<Napi::Function>());
        }
    
   +    // Create an own instance property.
   +    info.This().As<Napi::Object>().DefineProperty(
   +        Napi::PropertyDescriptor::Accessor(info.Env(),
   +                                           info.This().As<Napi::Object>(),
   +                                           "ownProperty",
   +                                           OwnPropertyGetter,
   +                                           napi_enumerable));
   +
   +    // Create an own instance property with a templated function.
   +    info.This().As<Napi::Object>().DefineProperty(
   +        Napi::PropertyDescriptor::Accessor<OwnPropertyGetter>("ownPropertyT",
   +                                                              napi_enumerable));
   +  }
   +
   +  static Napi::Value OwnPropertyGetter(const Napi::CallbackInfo& info) {
   +    return Napi::Number::New(info.Env(), 42);
      }
    
      void Setter(const Napi::CallbackInfo& /*info*/, const Napi::Value& value) {
   diff --git a/test/objectwrap.js b/test/objectwrap.js
   index de16a60..dd2b628 100644
   --- a/test/objectwrap.js
   +++ b/test/objectwrap.js
   @@ -72,6 +72,17 @@ const test = (binding) => {
          obj[clazz.kTestAccessorTInternal] = 'instance internal getset 4';
          assert.strictEqual(obj[clazz.kTestAccessorTInternal], 'instance internal getset 4');
        }
   +
   +    // own property
   +    {
   +      // Make sure the properties are enumerable.
   +      assert(Object.getOwnPropertyNames(obj).indexOf('ownProperty') >= 0);
   +      assert(Object.getOwnPropertyNames(obj).indexOf('ownPropertyT') >= 0);
   +
   +      // Make sure the properties return the right value.
   +      assert.strictEqual(obj.ownProperty, 42);
   +      assert.strictEqual(obj.ownPropertyT, 42);
   +    }
      };
    
      const testMethod = (obj, clazz) => {

   👍React with 👍1Reacted by gms1
6. gms1 commented on Jan 6, 2020

   

   gms1

   on Jan 6, 2020

   ContributorAuthor

   More actions

   many thanks! the test fails as it should:

   -    assert.deepEqual(Object.keys(obj), []);
   +    assert.deepEqual(Object.keys(obj), ['ownProperty', 'ownPropertyT']);
7. gabrielschulhof commented on Jan 7, 2020

   

   gabrielschulhof

   on Jan 7, 2020

   Contributor

   More actions

   @gms1 AFAICT #645 demonstrates that defining own properties as enumerable works. Thus, #645 doesn't fix this issue because this is not an issue. We'll keep and land #645 because it's a good test, but I don't think we can claim that it fixes this issue.
8. gabrielschulhof

   closed this as completedon Jan 7, 2020