Skip to content

Commit db36ca5

Browse files
jasonginMylesBorins
jasongin
authored and
MylesBorins
committed
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. Backport-PR-URL: #19447 PR-URL: #12240 Fixes: nodejs/abi-stable-node#221 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
1 parent 1e6d3bb commit db36ca5

File tree

6 files changed

+107
-66
lines changed

6 files changed

+107
-66
lines changed

‎src/node_api.cc‎

Lines changed: 57 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,26 @@ class napi_env__ {
3737
namespace v8impl {
3838

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

@@ -777,7 +785,7 @@ napi_status napi_define_class(napi_env env,
777785
for (size_t i = 0; i < property_count; i++) {
778786
const napi_property_descriptor* p = properties + i;
779787

780-
if ((p->attributes & napi_static_property) != 0) {
788+
if ((p->attributes & napi_static) != 0) {
781789
// Static properties are handled separately below.
782790
static_property_count++;
783791
continue;
@@ -786,25 +794,11 @@ napi_status napi_define_class(napi_env env,
786794
v8::Local<v8::String> property_name;
787795
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
788796
v8::PropertyAttribute attributes =
789-
v8impl::V8PropertyAttributesFromAttributes(p->attributes);
797+
v8impl::V8PropertyAttributesFromDescriptor(p);
790798

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

@@ -815,6 +809,20 @@ napi_status napi_define_class(napi_env env,
815809
cbdata,
816810
v8::AccessControl::DEFAULT,
817811
attributes);
812+
} else if (p->method != nullptr) {
813+
v8::Local<v8::Object> cbdata =
814+
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
815+
816+
RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
817+
818+
v8::Local<v8::FunctionTemplate> t =
819+
v8::FunctionTemplate::New(isolate,
820+
v8impl::FunctionCallbackWrapper::Invoke,
821+
cbdata,
822+
v8::Signature::New(isolate, tpl));
823+
t->SetClassName(property_name);
824+
825+
tpl->PrototypeTemplate()->Set(property_name, t, attributes);
818826
} else {
819827
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
820828
tpl->PrototypeTemplate()->Set(property_name, value, attributes);
@@ -829,7 +837,7 @@ napi_status napi_define_class(napi_env env,
829837

830838
for (size_t i = 0; i < property_count; i++) {
831839
const napi_property_descriptor* p = properties + i;
832-
if ((p->attributes & napi_static_property) != 0) {
840+
if ((p->attributes & napi_static) != 0) {
833841
static_descriptors.push_back(*p);
834842
}
835843
}
@@ -1096,10 +1104,28 @@ napi_status napi_define_properties(napi_env env,
10961104
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
10971105

10981106
v8::PropertyAttribute attributes =
1099-
v8impl::V8PropertyAttributesFromAttributes(
1100-
(napi_property_attributes)(p->attributes & ~napi_static_property));
1107+
v8impl::V8PropertyAttributesFromDescriptor(p);
1108+
1109+
if (p->getter != nullptr || p->setter != nullptr) {
1110+
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
1111+
env,
1112+
p->getter,
1113+
p->setter,
1114+
p->data);
11011115

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

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

‎src/node_api_types.h‎

Lines changed: 4 additions & 4 deletions
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‎

Lines changed: 15 additions & 1 deletion
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‎

Lines changed: 8 additions & 5 deletions
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‎

Lines changed: 15 additions & 1 deletion
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‎

Lines changed: 8 additions & 5 deletions
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)