Skip to content

Commit 05586f4

Browse files
jasonginjasongin
committed
Refactor attribute normalization, add tests
1 parent 44cef98 commit 05586f4

File tree

5 files changed

+95
-64
lines changed

5 files changed

+95
-64
lines changed

‎src/node_api.cc

+57-60
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/******************************************************************************
1+
/******************************************************************************
22
* Experimental prototype for demonstrating VM agnostic and ABI stable API
33
* for native modules to use instead of using Nan and V8 APIs directly.
44
*
@@ -35,18 +35,27 @@ 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_writable) == 0) {
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+
}
48+
else if ((descriptor->attributes & napi_writable) == 0) {
49+
attribute_flags |= v8::PropertyAttribute::ReadOnly;
4350
}
44-
if ((attributes & napi_enumerable) == 0) {
45-
attribute_flags |= v8::DontEnum;
51+
52+
if ((descriptor->attributes & napi_enumerable) == 0) {
53+
attribute_flags |= v8::PropertyAttribute::DontEnum;
4654
}
47-
if ((attributes & napi_configurable) == 0) {
48-
attribute_flags |= v8::DontDelete;
55+
if ((descriptor->attributes & napi_configurable) == 0) {
56+
attribute_flags |= v8::PropertyAttribute::DontDelete;
4957
}
58+
5059
return static_cast<v8::PropertyAttribute>(attribute_flags);
5160
}
5261

@@ -784,41 +793,35 @@ napi_status napi_define_class(napi_env env,
784793
v8::Local<v8::String> property_name;
785794
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
786795
v8::PropertyAttribute attributes =
787-
v8impl::V8PropertyAttributesFromAttributes(p->attributes);
796+
v8impl::V8PropertyAttributesFromDescriptor(p);
788797

789-
// This code is similar to that in napi_define_property(); the
798+
// This code is similar to that in napi_define_properties(); the
790799
// difference is it applies to a template instead of an object.
791-
if (p->method != nullptr) {
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 != nullptr || p->setter != nullptr) {
800+
if (p->getter != nullptr || p->setter != nullptr) {
806801
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
807802
env, p->getter, p->setter, p->data);
808803

809-
// The napi_writable attribute is ignored for accessor descriptors,
810-
// but V8 requires the ReadOnly attribute to match existence of a setter.
811-
attributes = static_cast<v8::PropertyAttribute>(p->setter != nullptr
812-
? attributes & ~v8::PropertyAttribute::ReadOnly
813-
: attributes | v8::PropertyAttribute::ReadOnly);
814-
815804
tpl->PrototypeTemplate()->SetAccessor(
816805
property_name,
817806
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
818807
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
819808
cbdata,
820809
v8::AccessControl::DEFAULT,
821810
attributes);
811+
} else if (p->method != nullptr) {
812+
v8::Local<v8::Object> cbdata =
813+
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
814+
815+
RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
816+
817+
v8::Local<v8::FunctionTemplate> t =
818+
v8::FunctionTemplate::New(isolate,
819+
v8impl::FunctionCallbackWrapper::Invoke,
820+
cbdata,
821+
v8::Signature::New(isolate, tpl));
822+
t->SetClassName(property_name);
823+
824+
tpl->PrototypeTemplate()->Set(property_name, t, attributes);
822825
} else {
823826
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
824827
tpl->PrototypeTemplate()->Set(property_name, value, attributes);
@@ -1100,9 +1103,28 @@ napi_status napi_define_properties(napi_env env,
11001103
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
11011104

11021105
v8::PropertyAttribute attributes =
1103-
v8impl::V8PropertyAttributesFromAttributes(p->attributes);
1106+
v8impl::V8PropertyAttributesFromDescriptor(p);
11041107

1105-
if (p->method != nullptr) {
1108+
if (p->getter != nullptr || p->setter != nullptr) {
1109+
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
1110+
env,
1111+
p->getter,
1112+
p->setter,
1113+
p->data);
1114+
1115+
auto set_maybe = obj->SetAccessor(
1116+
context,
1117+
name,
1118+
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
1119+
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
1120+
cbdata,
1121+
v8::AccessControl::DEFAULT,
1122+
attributes);
1123+
1124+
if (!set_maybe.FromMaybe(false)) {
1125+
return napi_set_last_error(env, napi_invalid_arg);
1126+
}
1127+
} else if (p->method != nullptr) {
11061128
v8::Local<v8::Object> cbdata =
11071129
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
11081130

@@ -1117,31 +1139,6 @@ napi_status napi_define_properties(napi_env env,
11171139
if (!define_maybe.FromMaybe(false)) {
11181140
return napi_set_last_error(env, napi_generic_failure);
11191141
}
1120-
} else if (p->getter != nullptr || p->setter != nullptr) {
1121-
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
1122-
env,
1123-
p->getter,
1124-
p->setter,
1125-
p->data);
1126-
1127-
// The napi_writable attribute is ignored for accessor descriptors,
1128-
// but V8 requires the ReadOnly attribute to match existence of a setter.
1129-
attributes = static_cast<v8::PropertyAttribute>(p->setter != nullptr
1130-
? attributes & ~v8::PropertyAttribute::ReadOnly
1131-
: attributes | v8::PropertyAttribute::ReadOnly);
1132-
1133-
auto set_maybe = obj->SetAccessor(
1134-
context,
1135-
name,
1136-
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
1137-
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
1138-
cbdata,
1139-
v8::AccessControl::DEFAULT,
1140-
attributes);
1141-
1142-
if (!set_maybe.FromMaybe(false)) {
1143-
return napi_set_last_error(env, napi_invalid_arg);
1144-
}
11451142
} else {
11461143
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
11471144

‎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

+4-1
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,13 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
8383

8484
napi_property_descriptor properties[] = {
8585
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
86-
{ "accessorValue", 0, GetValue, SetValue, 0, napi_enumerable, 0},
8786
{ "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
8887
{ "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
8988
{ "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

+4-1
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,13 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
7171

7272
napi_property_descriptor properties[] = {
7373
{ "echo", Echo, 0, 0, 0, napi_enumerable, 0 },
74-
{ "accessorValue", 0, GetValue, SetValue, 0, napi_enumerable, 0},
7574
{ "readwriteValue", 0, 0, 0, number, napi_enumerable | napi_writable, 0 },
7675
{ "readonlyValue", 0, 0, 0, number, napi_enumerable, 0},
7776
{ "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)