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

ObjectWrap: enumerable properties seem to be broken #644

Closed
gms1 opened this issue Jan 5, 2020 · 7 comments
Closed

ObjectWrap: enumerable properties seem to be broken #644

gms1 opened this issue Jan 5, 2020 · 7 comments

Comments

@gms1
Copy link
Contributor

gms1 commented Jan 5, 2020

in 'test/objectwrap.cc' the property 'testGetter' is defined as enumerable:

InstanceAccessor("testGetter", &Test::Getter, nullptr, napi_enumerable),

but testing if this property is enumerable:

gms@orion:~/work/RESEARCH/node-addon-api (master *)$ git diff
diff --git a/test/objectwrap.js b/test/objectwrap.js
index de16a60..e5d3efc 100644
--- a/test/objectwrap.js
+++ b/test/objectwrap.js
@@ -11,6 +11,10 @@ const test = (binding) => {
   };
 
   const testAccessor = (obj, clazz) => {
+    // enumerable
+    {
+      assert.strictEqual(obj.propertyIsEnumerable('testGetter'), true);
+    }
     // read-only, write-only
     {
       obj.testSetter = 'instance getter';

fails with:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true
@legendecas
Copy link
Member

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

Try:

assert.strictEqual(Object.getOwnPropertyDescriptor(Object.getPrototypeOf(obj), 'testGetter').enumerable, true);

@gms1
Copy link
Contributor Author

gms1 commented Jan 5, 2020

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

@gabrielschulhof
Copy link
Contributor

@gms1 you specify per-instance properties as a parameter to the DefineClass() call in your ObjectWrap subclass.

@gms1
Copy link
Contributor Author

gms1 commented Jan 6, 2020

@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.

@gabrielschulhof
Copy link
Contributor

@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) => {

@gms1
Copy link
Contributor Author

gms1 commented Jan 6, 2020

many thanks! the test fails as it should:

-    assert.deepEqual(Object.keys(obj), []);
+    assert.deepEqual(Object.keys(obj), ['ownProperty', 'ownPropertyT']);

@gabrielschulhof
Copy link
Contributor

@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.

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

Successfully merging a pull request may close this issue.

3 participants