Object spread not working with Napi::ObjectWrap

[pdehne]

Objects implemented using Napi::ObjectWrap do not seem to support the object spread syntax.

The following code on an object implemented with Napi::ObjectWrap always creates an empty object. The two properties are implemented as instance accessors using Napi.

const addonSampleObject = new addon.SampleObject();
addonSampleObject.stringProp = "Hello World";
addonSampleObject.numberProp = 3.1415926536;

const spreadAddonSampleObject = {...addonSampleObject};

This code with an object implemented using Nan::ObjectWrap works as expected, creating an object with the two properties that have the same values as the original object.

To make it easy to reproduce I have created two GitHub repositories, one with a Napi implementation and one with a NAN implementation.

Am I doing something wrong? Is this a known limitation or maybe a bug?

Any advice is very much appreciated.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mhdawson]

I spent some time looking at this today and I think the issue is that in these lines (EDIT, it was a different issue see below):

        SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp"),

the default attributes are used. Those are currently napi_default, for example:

    static PropertyDescriptor InstanceAccessor(Symbol name,
                                               napi_property_attributes attributes = napi_default,
                                               void* data = nullptr);

napi_default was defined as

typedef enum {
  napi_default = 0,
  napi_writable = 1 << 0,
  napi_enumerable = 1 << 1,
  napi_configurable = 1 << 2,

  // Used with napi_define_class to distinguish static properties
  // from instance properties. Ignored by napi_define_properties.
  napi_static = 1 << 10,

  // Default for class methods.
  napi_default_method = napi_writable | napi_configurable,

  // Default for object properties, like in JS obj[prop].
  napi_default_jsproperty = napi_writable |
                          napi_enumerable |
                          napi_configurable,
} napi_property_attributes;

The net being that the accessors will not be enumerable. If I remember correctly we figured out that napi_default was not a great default and added napi_default_method and napi_default_jsproperty.

We might want to change to using napi_default_jsproperty instead of napi_default as the defaults for InstanceAccessors. We'd have to decided if that is a breaking change or just a bug fix.

Will try out that change to see if what I think the problem is the problem or not using the sample code provided.

[mhdawson]

With the default changed, the accessors are now enumerable but the spread still does not seem to work. Not sure why not at this point. Had assumed not being enumerable was the problem. Maybe there is some other property that needs to be set.

[mhdawson]

Seems like another difference is that the properties with node-addon-api are not showing up as "own" properties.

[mhdawson]

Ok so I think I have the answer. We may need to improve the docs to explain the situation as I can see it would be easy to run into this issue.

The main issue is that using SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp") adds the property/accessor to the prototype. On the other hand spread, keys etc. only include own properties versus though inherited from the prototype chain.

The following patch coverts the code provided (not optimal, just re-using code that was there, could be reduced/refactored better) so that the properties/accessor is added to each object when the object is created. That makes them own properties and the test passes as expected.

diff --git a/src/SampleObject.cpp b/src/SampleObject.cpp
index 003c468..3f65c99 100644
--- a/src/SampleObject.cpp
+++ b/src/SampleObject.cpp
@@ -29,10 +29,42 @@ private:
     double m_number;
 };
 
+Napi::Value StringGetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  return holder->GetStringProp(info);
+}
+
+void StringSetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  holder->SetStringProp(info, info[0]);
+}
+
+Napi::Value NumberGetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  return holder->GetNumberProp(info);
+}
+
+void NumberSetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  holder->SetNumberProp(info, info[0]);
+}
+
 SampleObject::SampleObject(const Napi::CallbackInfo &info)
 : ObjectWrap(info)
 {
     m_native.reset(new SampleObjectNative());
+
+    // Create an own instance property.
+    info.This().As<Napi::Object>().DefineProperties({
+      Napi::PropertyDescriptor::Accessor<StringGetter, StringSetter>(
+             "stringProp",
+             napi_default_jsproperty,
+             (void*)this),
+      Napi::PropertyDescriptor::Accessor<NumberGetter, NumberSetter>(
+             "numberProp",
+             napi_default_jsproperty,
+             (void*)this)
+    });
 }
 
 Napi::Value SampleObject::GetStringProp(const Napi::CallbackInfo &info) {
@@ -57,7 +89,7 @@ void SampleObject::SetNumberProp(const Napi::CallbackInfo &info, const Napi::Val
 void SampleObject::Init(Napi::Env env, Napi::Object exports)
 {
     exports.Set("SampleObject", DefineClass(env, "SampleObject", {
-        SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp"),
-        SampleObject::InstanceAccessor<&SampleObject::GetNumberProp, &SampleObject::SetNumberProp>("numberProp"),
+//        SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp"),
+//        SampleObject::InstanceAccessor<&SampleObject::GetNumberProp, &SampleObject::SetNumberProp>("numberProp"),
     }));
 }

I'll discuss in the next node-appi team member to see if there is anything we can/want to change/improved in the code but I think at least some additions/clarifications in the docs/examples would be good.

@pdehne you should be able to use the approach I've shown in the patch to get the behaviour you were expecting.

[mhdawson]

@pdehne I'll also add that I know the answer may be too late for you, but may still helps others who run into the same issue.

[pdehne]

@pdehne I'll also add that I know the answer may be too late for you, but may still helps others who run into the same issue.

No worries, many thanks for looking into it! I'll take a look at your patch soon and let you know if this fixes the issue.

[pdehne]

@mhdawson I tried it, your patch works fine, many thank! This allows me to replace NAN with node-addon-api.

Having a more concise way to express "own" properties, would be great though. Wrapping classes with many members will be a bit tedious. For example having the ability to express own properties as part of DefineClass would make it a lot easier.

[mhdawson]

@pdehne great to hear it worked for you. @gabrielschulhof FYI in terms of thinking of how we might make this easier with DefineProperties.

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[lili2012]

Ok so I think I have the answer. We may need to improve the docs to explain the situation as I can see it would be easy to run into this issue.

The main issue is that using SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp") adds the property/accessor to the prototype. On the other hand spread, keys etc. only include own properties versus though inherited from the prototype chain.

The following patch coverts the code provided (not optimal, just re-using code that was there, could be reduced/refactored better) so that the properties/accessor is added to each object when the object is created. That makes them own properties and the test passes as expected.

diff --git a/src/SampleObject.cpp b/src/SampleObject.cpp
index 003c468..3f65c99 100644
--- a/src/SampleObject.cpp
+++ b/src/SampleObject.cpp
@@ -29,10 +29,42 @@ private:
     double m_number;
 };
 
+Napi::Value StringGetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  return holder->GetStringProp(info);
+}
+
+void StringSetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  holder->SetStringProp(info, info[0]);
+}
+
+Napi::Value NumberGetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  return holder->GetNumberProp(info);
+}
+
+void NumberSetter(const Napi::CallbackInfo& info) {
+  SampleObject* holder = reinterpret_cast<SampleObject*>(info.Data());
+  holder->SetNumberProp(info, info[0]);
+}
+
 SampleObject::SampleObject(const Napi::CallbackInfo &info)
 : ObjectWrap(info)
 {
     m_native.reset(new SampleObjectNative());
+
+    // Create an own instance property.
+    info.This().As<Napi::Object>().DefineProperties({
+      Napi::PropertyDescriptor::Accessor<StringGetter, StringSetter>(
+             "stringProp",
+             napi_default_jsproperty,
+             (void*)this),
+      Napi::PropertyDescriptor::Accessor<NumberGetter, NumberSetter>(
+             "numberProp",
+             napi_default_jsproperty,
+             (void*)this)
+    });
 }
 
 Napi::Value SampleObject::GetStringProp(const Napi::CallbackInfo &info) {
@@ -57,7 +89,7 @@ void SampleObject::SetNumberProp(const Napi::CallbackInfo &info, const Napi::Val
 void SampleObject::Init(Napi::Env env, Napi::Object exports)
 {
     exports.Set("SampleObject", DefineClass(env, "SampleObject", {
-        SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp"),
-        SampleObject::InstanceAccessor<&SampleObject::GetNumberProp, &SampleObject::SetNumberProp>("numberProp"),
+//        SampleObject::InstanceAccessor<&SampleObject::GetStringProp, &SampleObject::SetStringProp>("stringProp"),
+//        SampleObject::InstanceAccessor<&SampleObject::GetNumberProp, &SampleObject::SetNumberProp>("numberProp"),
     }));
 }

I'll discuss in the next node-appi team member to see if there is anything we can/want to change/improved in the code but I think at least some additions/clarifications in the docs/examples would be good.

@pdehne you should be able to use the approach I've shown in the patch to get the behaviour you were expecting.

@mhdawson I tried your patch and got the following output:

const addon = require("../lib/binding.js");
const util = require('node:util');

const addonSampleObject = new addon.SampleObject();

addonSampleObject.stringProp = "Hello World";
addonSampleObject.numberProp = 3.1415926536;

const spreadAddonSampleObject = {...addonSampleObject};

console.log(addonSampleObject)
// SampleObject {
//     stringProp: [Getter/Setter],
//     numberProp: [Getter/Setter]
// }

console.log(JSON.stringify(addonSampleObject))
//{"stringProp":"Hello World","numberProp":3.1415926536}


console.log(spreadAddonSampleObject)
//{ stringProp: 'Hello World', numberProp: 3.1415926536 }

console.log(util.inspect(addonSampleObject, { showHidden: true, depth: null }));
// SampleObject {
//     stringProp: [Getter/Setter],
//     numberProp: [Getter/Setter]
// }

could console.log(addonSampleObject) output like : { stringProp: 'Hello World', numberProp: 3.1415926536 } instead of // SampleObject { // stringProp: [Getter/Setter], // numberProp: [Getter/Setter] // }

Thanks!

[KevinEady]

Hi @lili2012 ,

util.inspect has an additional configuration parameter for getters: when set to true, getters will be inspected. See util.inspect documentation.

Can you add getters: true to your util.inspect call? Does this help?

[lili2012]

@KevinEady Thanks I tried your method. But I want to know could console.log(addonSampleObject) output { stringProp: 'Hello World', numberProp: 3.1415926536 } without using JSON.stringify or util.inspect. How could I achieve that with nodeaddon.

[KevinEady]

Hi @lili2012 ,

The original issue was only discussing how to enumerate the properties correctly for use with object spread. console.log internally uses util.inspect on objects to display them. If you want to use console.log() and show your object's getters without using util.inspect, I can think of two ways:

1. Assign a property [util.inspect.custom] to your object which will get called when using console.log and return the string you want to display to stdout. See documentation, StackOverflow example.

2. Overwrite the console global with a new Console that changes the default inspect options or create a new console specifically for logging your objects.

globalThis.console = new Console({ stdout: process.stdout, stderr: process.stderr, inspectOptions: { getters: true } });
console.log(addonSampleObject);
/*
SampleObject {
  stringProp: [Getter/Setter: 'Hello World'],
  numberProp: [Getter/Setter: 3.1415926536]
}
*/

[github-actions[bot]]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[UrielCh]

Hi, I have issue, with keys enumerations.

Is the enumeration of ObjectWrap fixed?

I'm mapping an openCV Mat with the following:

Napi::FunctionReference cvMatObject::constructor;

Napi::Object cvMatObject::Init(Napi::Env env, Napi::Object exports)
{
    Napi::HandleScope scope(env);
    napi_property_attributes napi_default_method = static_cast<napi_property_attributes>(napi_writable | napi_configurable | napi_enumerable);
    napi_property_attributes napi_default_Field = static_cast<napi_property_attributes>(napi_enumerable | napi_configurable);
    Napi::Function func = DefineClass(env, "cvMatObject", {
        // old syntax
        // InstanceAccessor("rows", &cvMatObject::Rows, &cvMatObject::ReadOnlySetter, napi_default_Field),
        // new syntax ?
        cvMatObject::InstanceAccessor<&cvMatObject::Rows, &cvMatObject::ReadOnlySetter>("rows", napi_default_Field),
        InstanceAccessor("cols", &cvMatObject::Cols, &cvMatObject::ReadOnlySetter, napi_default_Field),
        InstanceAccessor("type", &cvMatObject::Type, &cvMatObject::ReadOnlySetter, napi_default_Field),
        InstanceAccessor("depth", &cvMatObject::Depth, &cvMatObject::ReadOnlySetter, napi_default_Field),
        InstanceAccessor("channels", &cvMatObject::Channels, &cvMatObject::ReadOnlySetter, napi_default_Field),

        InstanceAccessor("isContinuous", &cvMatObject::IsContinuous, &cvMatObject::ReadOnlySetter),
        InstanceAccessor("isSubmatrix", &cvMatObject::IsSubmatrix, &cvMatObject::ReadOnlySetter),
        InstanceAccessor("elemSize", &cvMatObject::ElemSize, &cvMatObject::ReadOnlySetter),
        InstanceAccessor("elemSize1", &cvMatObject::ElemSize1, &cvMatObject::ReadOnlySetter),
        InstanceMethod("toString", &cvMatObject::ToString),
        });

    constructor = Napi::Persistent(func);
    constructor.SuppressDestruct();

    exports.Set("cvMatObject", func);
    return exports;
}

Later on, I'm testing this code:

    const logo = theModule.imread('./data/logo.png')
    console.log("logo cols:", logo.cols, " rows:", logo.rows, " type:", logo.type)
    console.log("-----------")
    console.log("logo:", logo)
    console.log("Object.keys(logo) return:", Object.keys(logo))
    console.log("logo:", logo.toString())

    try {
        // @ts-expect-error write a read-only property
        logo.rows = 1;
    } catch (e) {
        console.log("logo.rows is read-only Throw Ok");
    }

In the final output, I get that:

logo cols: 100  rows: 132  type: 16
-----------
logo: cvMatObject {}
Object.keys(logo) return: []
logo: Matrix: 132x100 type:16 Channels:3 ElemSize:3 ElemSize1:1
logo.rows is readonly Throw Ok

Here Object.keys(logo) return [], so no none of my attributes was enumerated.

May I get some assistance?

[KevinEady]

Hi @UrielCh ,

This is not an issue with node-addon-api and does not need to be "fixed", as it is the way the JavaScript works.

class Foo { get accessor() { return "x"; } }; 
object = new Foo();

object.accessor; // 'x'
Object.keys(object); // []

As stated in https://github.com/nodejs/node-addon-api/issues/1120#issuecomment-1118959208:

The main issue is that using InstanceAccessor adds the property/accessor to the prototype. On the other hand spread, keys etc. only include own properties versus though inherited from the prototype chain.

In order to make the accessor an own-property of the object, you can use DefineProperties like in the comment linked above, doing the C++ equivalent of:

class Foo { constructor() { Object.defineProperty(this,"accessor", { get: () => "x", enumerable: true } ) } }; 
object = new Foo();

object.accessor; // 'x'
Object.keys(object); // ['accessor']

Closing issue.

[UrielCh]

Thx, I will update my code soon.