Skip to content

Commit 8460284

Browse files
jasonginmhdawson
authored andcommitted
n-api: Update property attrs enum to match JS spec
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. PR-URL: #12240 Fixes: nodejs/abi-stable-node#221 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
1 parent 943d085 commit 8460284

File tree

6 files changed

+107
-66
lines changed

6 files changed

+107
-66
lines changed

src/node_api.cc

+57-50
Original file line numberDiff line numberDiff line change
@@ -35,18 +35,26 @@ class napi_env__ {
3535
namespace v8impl {
3636

3737
// convert from n-api property attributes to v8::PropertyAttribute
38-
static inline v8::PropertyAttribute V8PropertyAttributesFromAttributes(
39-
napi_property_attributes attributes) {
40-
unsigned int attribute_flags = v8::None;
41-
if (attributes & napi_read_only) {
42-
attribute_flags |= v8::ReadOnly;
38+
static inline v8::PropertyAttribute V8PropertyAttributesFromDescriptor(
39+
const napi_property_descriptor* descriptor) {
40+
unsigned int attribute_flags = v8::PropertyAttribute::None;
41+
42+
if (descriptor->getter != nullptr || descriptor->setter != nullptr) {
43+
// The napi_writable attribute is ignored for accessor descriptors, but
44+
// V8 requires the ReadOnly attribute to match nonexistence of a setter.
45+
attribute_flags |= (descriptor->setter == nullptr ?
46+
v8::PropertyAttribute::ReadOnly : v8::PropertyAttribute::None);
47+
} else if ((descriptor->attributes & napi_writable) == 0) {
48+
attribute_flags |= v8::PropertyAttribute::ReadOnly;
4349
}
44-
if (attributes & napi_dont_enum) {
45-
attribute_flags |= v8::DontEnum;
50+
51+
if ((descriptor->attributes & napi_enumerable) == 0) {
52+
attribute_flags |= v8::PropertyAttribute::DontEnum;
4653
}
47-
if (attributes & napi_dont_delete) {
48-
attribute_flags |= v8::DontDelete;
54+
if ((descriptor->attributes & napi_configurable) == 0) {
55+
attribute_flags |= v8::PropertyAttribute::DontDelete;
4956
}
57+
5058
return static_cast<v8::PropertyAttribute>(attribute_flags);
5159
}
5260

@@ -775,7 +783,7 @@ napi_status napi_define_class(napi_env env,
775783
for (size_t i = 0; i < property_count; i++) {
776784
const napi_property_descriptor* p = properties + i;
777785

778-
if ((p->attributes & napi_static_property) != 0) {
786+
if ((p->attributes & napi_static) != 0) {
779787
// Static properties are handled separately below.
780788
static_property_count++;
781789
continue;
@@ -784,25 +792,11 @@ napi_status napi_define_class(napi_env env,
784792
v8::Local<v8::String> property_name;
785793
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
786794
v8::PropertyAttribute attributes =
787-
v8impl::V8PropertyAttributesFromAttributes(p->attributes);
795+
v8impl::V8PropertyAttributesFromDescriptor(p);
788796

789-
// This code is similar to that in napi_define_property(); the
797+
// This code is similar to that in napi_define_properties(); the
790798
// difference is it applies to a template instead of an object.
791-
if (p->method) {
792-
v8::Local<v8::Object> cbdata =
793-
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
794-
795-
RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
796-
797-
v8::Local<v8::FunctionTemplate> t =
798-
v8::FunctionTemplate::New(isolate,
799-
v8impl::FunctionCallbackWrapper::Invoke,
800-
cbdata,
801-
v8::Signature::New(isolate, tpl));
802-
t->SetClassName(property_name);
803-
804-
tpl->PrototypeTemplate()->Set(property_name, t, attributes);
805-
} else if (p->getter || p->setter) {
799+
if (p->getter != nullptr || p->setter != nullptr) {
806800
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
807801
env, p->getter, p->setter, p->data);
808802

@@ -813,6 +807,20 @@ napi_status napi_define_class(napi_env env,
813807
cbdata,
814808
v8::AccessControl::DEFAULT,
815809
attributes);
810+
} else if (p->method != nullptr) {
811+
v8::Local<v8::Object> cbdata =
812+
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
813+
814+
RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
815+
816+
v8::Local<v8::FunctionTemplate> t =
817+
v8::FunctionTemplate::New(isolate,
818+
v8impl::FunctionCallbackWrapper::Invoke,
819+
cbdata,
820+
v8::Signature::New(isolate, tpl));
821+
t->SetClassName(property_name);
822+
823+
tpl->PrototypeTemplate()->Set(property_name, t, attributes);
816824
} else {
817825
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
818826
tpl->PrototypeTemplate()->Set(property_name, value, attributes);
@@ -827,7 +835,7 @@ napi_status napi_define_class(napi_env env,
827835

828836
for (size_t i = 0; i < property_count; i++) {
829837
const napi_property_descriptor* p = properties + i;
830-
if ((p->attributes & napi_static_property) != 0) {
838+
if ((p->attributes & napi_static) != 0) {
831839
static_descriptors.push_back(*p);
832840
}
833841
}
@@ -1094,10 +1102,28 @@ napi_status napi_define_properties(napi_env env,
10941102
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
10951103

10961104
v8::PropertyAttribute attributes =
1097-
v8impl::V8PropertyAttributesFromAttributes(
1098-
(napi_property_attributes)(p->attributes & ~napi_static_property));
1105+
v8impl::V8PropertyAttributesFromDescriptor(p);
1106+
1107+
if (p->getter != nullptr || p->setter != nullptr) {
1108+
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
1109+
env,
1110+
p->getter,
1111+
p->setter,
1112+
p->data);
10991113

1100-
if (p->method) {
1114+
auto set_maybe = obj->SetAccessor(
1115+
context,
1116+
name,
1117+
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
1118+
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
1119+
cbdata,
1120+
v8::AccessControl::DEFAULT,
1121+
attributes);
1122+
1123+
if (!set_maybe.FromMaybe(false)) {
1124+
return napi_set_last_error(env, napi_invalid_arg);
1125+
}
1126+
} else if (p->method != nullptr) {
11011127
v8::Local<v8::Object> cbdata =
11021128
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
11031129

@@ -1112,25 +1138,6 @@ napi_status napi_define_properties(napi_env env,
11121138
if (!define_maybe.FromMaybe(false)) {
11131139
return napi_set_last_error(env, napi_generic_failure);
11141140
}
1115-
} else if (p->getter || p->setter) {
1116-
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
1117-
env,
1118-
p->getter,
1119-
p->setter,
1120-
p->data);
1121-
1122-
auto set_maybe = obj->SetAccessor(
1123-
context,
1124-
name,
1125-
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
1126-
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
1127-
cbdata,
1128-
v8::AccessControl::DEFAULT,
1129-
attributes);
1130-
1131-
if (!set_maybe.FromMaybe(false)) {
1132-
return napi_set_last_error(env, napi_invalid_arg);
1133-
}
11341141
} else {
11351142
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
11361143

src/node_api_types.h

+4-4
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@ typedef void (*napi_finalize)(napi_env env,
2525

2626
typedef enum {
2727
napi_default = 0,
28-
napi_read_only = 1 << 0,
29-
napi_dont_enum = 1 << 1,
30-
napi_dont_delete = 1 << 2,
28+
napi_writable = 1 << 0,
29+
napi_enumerable = 1 << 1,
30+
napi_configurable = 1 << 2,
3131

3232
// Used with napi_define_class to distinguish static properties
3333
// from instance properties. Ignored by napi_define_properties.
34-
napi_static_property = 1 << 10,
34+
napi_static = 1 << 10,
3535
} napi_property_attributes;
3636

3737
typedef struct {

test/addons-napi/test_constructor/test.js

+15-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ assert.throws(() => { test_object.readonlyValue = 3; });
1717

1818
assert.ok(test_object.hiddenValue);
1919

20-
// All properties except 'hiddenValue' should be enumerable.
20+
// Properties with napi_enumerable attribute should be enumerable.
2121
const propertyNames = [];
2222
for (const name in test_object) {
2323
propertyNames.push(name);
@@ -26,3 +26,17 @@ assert.ok(propertyNames.indexOf('echo') >= 0);
2626
assert.ok(propertyNames.indexOf('readwriteValue') >= 0);
2727
assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
2828
assert.ok(propertyNames.indexOf('hiddenValue') < 0);
29+
assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
30+
assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
31+
assert.ok(propertyNames.indexOf('readonlyAccessor1') < 0);
32+
assert.ok(propertyNames.indexOf('readonlyAccessor2') < 0);
33+
34+
// The napi_writable attribute should be ignored for accessors.
35+
test_object.readwriteAccessor1 = 1;
36+
assert.strictEqual(test_object.readwriteAccessor1, 1);
37+
assert.strictEqual(test_object.readonlyAccessor1, 1);
38+
assert.throws(() => { test_object.readonlyAccessor1 = 3; });
39+
test_object.readwriteAccessor2 = 2;
40+
assert.strictEqual(test_object.readwriteAccessor2, 2);
41+
assert.strictEqual(test_object.readonlyAccessor2, 2);
42+
assert.throws(() => { test_object.readonlyAccessor2 = 3; });

test/addons-napi/test_constructor/test_constructor.c

+8-5
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,14 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
8282
if (status != napi_ok) return;
8383

8484
napi_property_descriptor properties[] = {
85-
{ "echo", Echo, 0, 0, 0, napi_default, 0 },
86-
{ "accessorValue", 0, GetValue, SetValue, 0, napi_default, 0},
87-
{ "readwriteValue", 0, 0, 0, number, napi_default, 0 },
88-
{ "readonlyValue", 0, 0, 0, number, napi_read_only, 0},
89-
{ "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0},
85+
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
86+
{ "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
87+
{ "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
88+
{ "hiddenValue", 0, 0, 0, number, napi_default, 0},
89+
{ "readwriteAccessor1", 0, GetValue, SetValue, 0, napi_default, 0},
90+
{ "readwriteAccessor2", 0, GetValue, SetValue, 0, napi_writable, 0},
91+
{ "readonlyAccessor1", 0, GetValue, NULL, 0, napi_default, 0},
92+
{ "readonlyAccessor2", 0, GetValue, NULL, 0, napi_writable, 0},
9093
};
9194

9295
napi_value cons;

test/addons-napi/test_properties/test.js

+15-1
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ assert.throws(() => { test_object.readonlyValue = 3; });
1616

1717
assert.ok(test_object.hiddenValue);
1818

19-
// All properties except 'hiddenValue' should be enumerable.
19+
// Properties with napi_enumerable attribute should be enumerable.
2020
const propertyNames = [];
2121
for (const name in test_object) {
2222
propertyNames.push(name);
@@ -25,3 +25,17 @@ assert.ok(propertyNames.indexOf('echo') >= 0);
2525
assert.ok(propertyNames.indexOf('readwriteValue') >= 0);
2626
assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
2727
assert.ok(propertyNames.indexOf('hiddenValue') < 0);
28+
assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
29+
assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
30+
assert.ok(propertyNames.indexOf('readonlyAccessor1') < 0);
31+
assert.ok(propertyNames.indexOf('readonlyAccessor2') < 0);
32+
33+
// The napi_writable attribute should be ignored for accessors.
34+
test_object.readwriteAccessor1 = 1;
35+
assert.strictEqual(test_object.readwriteAccessor1, 1);
36+
assert.strictEqual(test_object.readonlyAccessor1, 1);
37+
assert.throws(() => { test_object.readonlyAccessor1 = 3; });
38+
test_object.readwriteAccessor2 = 2;
39+
assert.strictEqual(test_object.readwriteAccessor2, 2);
40+
assert.strictEqual(test_object.readonlyAccessor2, 2);
41+
assert.throws(() => { test_object.readonlyAccessor2 = 3; });

test/addons-napi/test_properties/test_properties.c

+8-5
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,14 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
7070
if (status != napi_ok) return;
7171

7272
napi_property_descriptor properties[] = {
73-
{ "echo", Echo, 0, 0, 0, napi_default, 0 },
74-
{ "accessorValue", 0, GetValue, SetValue, 0, napi_default, 0 },
75-
{ "readwriteValue", 0, 0, 0, number, napi_default, 0 },
76-
{ "readonlyValue", 0, 0, 0, number, napi_read_only, 0 },
77-
{ "hiddenValue", 0, 0, 0, number, napi_read_only | napi_dont_enum, 0 },
73+
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
74+
{ "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
75+
{ "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
76+
{ "hiddenValue", 0, 0, 0, number, napi_default, 0},
77+
{ "readwriteAccessor1", 0, GetValue, SetValue, 0, napi_default, 0},
78+
{ "readwriteAccessor2", 0, GetValue, SetValue, 0, napi_writable, 0},
79+
{ "readonlyAccessor1", 0, GetValue, NULL, 0, napi_default, 0},
80+
{ "readonlyAccessor2", 0, GetValue, NULL, 0, napi_writable, 0},
7881
};
7982

8083
status = napi_define_properties(

0 commit comments

Comments
 (0)