Skip to content
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions doc/addon.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@ class ExampleAddon : public Napi::Addon<ExampleAddon> {

// We can also attach plain objects to `exports`, and instance methods as
// properties of those sub-objects.
InstanceValue("subObject", DefineProperties(Napi::Object::New(), {
InstanceMethod("decrement", &ExampleAddon::Decrement
})), napi_enumerable)
InstanceValue("subObject", DefineProperties(Napi::Object::New(env), {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand adding the env to New() but not removing napi_enumerable as the signature seems like it requires that:

inline ClassPropertyDescriptor<T> InstanceWrap<T>::InstanceValue(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to InstanceWrap doc:

template <typename T>
static Napi::ClassPropertyDescriptor<T>
Napi::InstanceWrap<T>::InstanceValue(const char* utf8name,
                            Napi::Value value,
                            napi_property_attributes attributes = napi_default);

napi_enumerable can be set but it is not required. I have verified that this code works.

But if you think that napi_enumerable needs to be included into this example then it still needs to be fixed because of wrong position of brackets:

InstanceValue("subObject", DefineProperties(Napi::Object::New(env), {
    InstanceMethod("decrement", &ExampleAddon::Decrement) // <- missing bracket
})/*) <- wrong bracket */, napi_enumerable)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without napi_enumerable the items cannot be seen from JS without some effort. Please leave it in place! Everything else LGTM.

Copy link
Copy Markdown
Contributor Author

@nempoBu4 nempoBu4 Oct 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielschulhof I thought napi_enumerable means this items can be seen through for..in iteration or Object.keys as stated in the MDN's Enumerability and ownership of properties article. I thought without napi_enumerable the property still can be accessed through dot (obj.property) or brackets (obj['property']) or destructuring assignment ({ property } = obj) and there is no harm to remove napi_enumerable because there is no for..in iteration in addon.md.

So, I was wrong. I put napi_enumerable back in place. Does it mean that napi_enumerable needs to be added to other items too?

DefineAddon(exports, {
  InstanceMethod("increment", &ExampleAddon::Increment, napi_enumerable), // <- here

  // We can also attach plain objects to `exports`, and instance methods as
  // properties of those sub-objects.
  InstanceValue("subObject", DefineProperties(Napi::Object::New(env), {
    InstanceMethod("decrement", &ExampleAddon::Decrement, napi_enumerable) // <- and here
  }), napi_enumerable)
});

InstanceMethod("decrement", &ExampleAddon::Decrement)
}))
});
}
private:
Expand Down