Skip to content

Commit

Permalink
n-api: Update property attrs enum to match JS spec
Browse files Browse the repository at this point in the history 
The napi_property_attributes enum used names and values from
v8::PropertyAttribute, but those negative flag names were outdated
along with the default behavior of a property being writable,
enumerable, and configurable unless otherwise specified. To match the
ES5 standard property descriptor those attributes should be positive
flags and should default to false unless otherwise specified.

Fixes: nodejs/abi-stable-node#221
  • Loading branch information
@jasongin
jasongin committed Apr 6, 2017
1 parent a94a5da commit 44cef98
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 25 deletions.
33 changes: 22 additions & 11 deletions src/node_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,13 @@ namespace v8impl {
static inline v8::PropertyAttribute V8PropertyAttributesFromAttributes(
napi_property_attributes attributes) {
unsigned int attribute_flags = v8::None;
if (attributes & napi_read_only) {
if ((attributes & napi_writable) == 0) {
attribute_flags |= v8::ReadOnly;
}
if (attributes & napi_dont_enum) {
if ((attributes & napi_enumerable) == 0) {
attribute_flags |= v8::DontEnum;
}
if (attributes & napi_dont_delete) {
if ((attributes & napi_configurable) == 0) {
attribute_flags |= v8::DontDelete;
}
return static_cast<v8::PropertyAttribute>(attribute_flags);
Expand Down Expand Up @@ -775,7 +775,7 @@ napi_status napi_define_class(napi_env env,
for (size_t i = 0; i < property_count; i++) {
const napi_property_descriptor* p = properties + i;

if ((p->attributes & napi_static_property) != 0) {
if ((p->attributes & napi_static) != 0) {
// Static properties are handled separately below.
static_property_count++;
continue;
Expand All @@ -788,7 +788,7 @@ napi_status napi_define_class(napi_env env,

// This code is similar to that in napi_define_property(); the
// difference is it applies to a template instead of an object.
if (p->method) {
if (p->method != nullptr) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);

Expand All @@ -802,10 +802,16 @@ napi_status napi_define_class(napi_env env,
t->SetClassName(property_name);

tpl->PrototypeTemplate()->Set(property_name, t, attributes);
} else if (p->getter || p->setter) {
} else if (p->getter != nullptr || p->setter != nullptr) {
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
env, p->getter, p->setter, p->data);

// The napi_writable attribute is ignored for accessor descriptors,
// but V8 requires the ReadOnly attribute to match existence of a setter.
attributes = static_cast<v8::PropertyAttribute>(p->setter != nullptr
? attributes & ~v8::PropertyAttribute::ReadOnly
: attributes | v8::PropertyAttribute::ReadOnly);

tpl->PrototypeTemplate()->SetAccessor(
property_name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
Expand All @@ -827,7 +833,7 @@ napi_status napi_define_class(napi_env env,

for (size_t i = 0; i < property_count; i++) {
const napi_property_descriptor* p = properties + i;
if ((p->attributes & napi_static_property) != 0) {
if ((p->attributes & napi_static) != 0) {
static_descriptors.push_back(*p);
}
}
Expand Down Expand Up @@ -1094,10 +1100,9 @@ napi_status napi_define_properties(napi_env env,
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);

v8::PropertyAttribute attributes =
v8impl::V8PropertyAttributesFromAttributes(
(napi_property_attributes)(p->attributes & ~napi_static_property));
v8impl::V8PropertyAttributesFromAttributes(p->attributes);

if (p->method) {
if (p->method != nullptr) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);

Expand All @@ -1112,13 +1117,19 @@ napi_status napi_define_properties(napi_env env,
if (!define_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_generic_failure);
}
} else if (p->getter || p->setter) {
} else if (p->getter != nullptr || p->setter != nullptr) {
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
env,
p->getter,
p->setter,
p->data);

// The napi_writable attribute is ignored for accessor descriptors,
// but V8 requires the ReadOnly attribute to match existence of a setter.
attributes = static_cast<v8::PropertyAttribute>(p->setter != nullptr
? attributes & ~v8::PropertyAttribute::ReadOnly
: attributes | v8::PropertyAttribute::ReadOnly);

auto set_maybe = obj->SetAccessor(
context,
name,
Expand Down
8 changes: 4 additions & 4 deletions src/node_api_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ typedef void (*napi_finalize)(napi_env env,

typedef enum {
napi_default = 0,
napi_read_only = 1 << 0,
napi_dont_enum = 1 << 1,
napi_dont_delete = 1 << 2,
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_property = 1 << 10,
napi_static = 1 << 10,
} napi_property_attributes;

typedef struct {
Expand Down
10 changes: 5 additions & 5 deletions test/addons-napi/test_constructor/test_constructor.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,11 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
if (status != napi_ok) return;

napi_property_descriptor properties[] = {
{ "echo", Echo, 0, 0, 0, napi_default, 0 },
{ "accessorValue", 0, GetValue, SetValue, 0, napi_default, 0},
{ "readwriteValue", 0, 0, 0, number, napi_default, 0 },
{ "readonlyValue", 0, 0, 0, number, napi_read_only, 0},
{ "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0},
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
{ "accessorValue", 0, GetValue, SetValue, 0, napi_enumerable, 0},
{ "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
{ "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
{ "hiddenValue", 0, 0, 0, number, napi_default, 0},
};

napi_value cons;
Expand Down
10 changes: 5 additions & 5 deletions test/addons-napi/test_properties/test_properties.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,11 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
if (status != napi_ok) return;

napi_property_descriptor properties[] = {
{ "echo", Echo, 0, 0, 0, napi_default, 0 },
{ "accessorValue", 0, GetValue, SetValue, 0, napi_default, 0 },
{ "readwriteValue", 0, 0, 0, number, napi_default, 0 },
{ "readonlyValue", 0, 0, 0, number, napi_read_only, 0 },
{ "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0 },
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
{ "accessorValue", 0, GetValue, SetValue, 0, napi_enumerable, 0},
{ "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
{ "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
{ "hiddenValue", 0, 0, 0, number, napi_default, 0},
};

status = napi_define_properties(
Expand Down

0 comments on commit 44cef98

Please sign in to comment.