Fixed example in addon.md. #820
Conversation
| InstanceValue("subObject", DefineProperties(Napi::Object::New(), { | ||
| InstanceMethod("decrement", &ExampleAddon::Decrement | ||
| })), napi_enumerable) | ||
| InstanceValue("subObject", DefineProperties(Napi::Object::New(env), { |
There was a problem hiding this comment.
I understand adding the env to New() but not removing napi_enumerable as the signature seems like it requires that:
Line 3425 in ceb27d4
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Without napi_enumerable the items cannot be seen from JS without some effort. Please leave it in place! Everything else LGTM.
There was a problem hiding this comment.
@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)
});
PR-URL: nodejs#820 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#820 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#820 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#820 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#820 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]>
No description provided.