lukechilds
/
node
mirror of https://github.com/lukechilds/node.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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: https://github.com/nodejs/node/pull/12240
Fixes: https://github.com/nodejs/abi-stable-node/issues/221
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
v6
Jason Ginchereau 8 years ago
committed by Michael Dawson
parent
943d085307
commit
84602845c6
6 changed files with 107 additions and 66 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 93
      src/node_api.cc
  2. 8
      src/node_api_types.h
  3. 16
      test/addons-napi/test_constructor/test.js
  4. 13
      test/addons-napi/test_constructor/test_constructor.c
  5. 16
      test/addons-napi/test_properties/test.js
  6. 13
      test/addons-napi/test_properties/test_properties.c

93
src/node_api.cc
View File

@ -35,18 +35,26 @@ class napi_env__ {
namespace v8impl {
// convert from n-api property attributes to v8::PropertyAttribute
static inline v8::PropertyAttribute V8PropertyAttributesFromAttributes(
napi_property_attributes attributes) {
unsigned int attribute_flags = v8::None;
if (attributes & napi_read_only) {
attribute_flags |= v8::ReadOnly;
static inline v8::PropertyAttribute V8PropertyAttributesFromDescriptor(
const napi_property_descriptor* descriptor) {
unsigned int attribute_flags = v8::PropertyAttribute::None;
if (descriptor->getter != nullptr || descriptor->setter != nullptr) {
// The napi_writable attribute is ignored for accessor descriptors, but
// V8 requires the ReadOnly attribute to match nonexistence of a setter.
attribute_flags |= (descriptor->setter == nullptr ?
v8::PropertyAttribute::ReadOnly : v8::PropertyAttribute::None);
} else if ((descriptor->attributes & napi_writable) == 0) {
attribute_flags |= v8::PropertyAttribute::ReadOnly;
}
if (attributes & napi_dont_enum) {
attribute_flags |= v8::DontEnum;
if ((descriptor->attributes & napi_enumerable) == 0) {
attribute_flags |= v8::PropertyAttribute::DontEnum;
}
if (attributes & napi_dont_delete) {
attribute_flags |= v8::DontDelete;
if ((descriptor->attributes & napi_configurable) == 0) {
attribute_flags |= v8::PropertyAttribute::DontDelete;
}
return static_cast<v8::PropertyAttribute>(attribute_flags);
}
@ -775,7 +783,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;
@ -784,11 +792,22 @@ napi_status napi_define_class(napi_env env,
v8::Local<v8::String> property_name;
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
v8::PropertyAttribute attributes =
v8impl::V8PropertyAttributesFromAttributes(p->attributes);
v8impl::V8PropertyAttributesFromDescriptor(p);
// This code is similar to that in napi_define_property(); the
// This code is similar to that in napi_define_properties(); the
// difference is it applies to a template instead of an object.
if (p->method) {
if (p->getter != nullptr || p->setter != nullptr) {
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
env, p->getter, p->setter, p->data);
tpl->PrototypeTemplate()->SetAccessor(
property_name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
cbdata,
v8::AccessControl::DEFAULT,
attributes);
} else if (p->method != nullptr) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
@ -802,17 +821,6 @@ 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) {
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
env, p->getter, p->setter, p->data);
tpl->PrototypeTemplate()->SetAccessor(
property_name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
cbdata,
v8::AccessControl::DEFAULT,
attributes);
} else {
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
tpl->PrototypeTemplate()->Set(property_name, value, attributes);
@ -827,7 +835,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);
}
}
@ -1094,25 +1102,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));
if (p->method) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
v8impl::V8PropertyAttributesFromDescriptor(p);
RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
auto define_maybe =
obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
if (!define_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_generic_failure);
}
} else if (p->getter || p->setter) {
if (p->getter != nullptr || p->setter != nullptr) {
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
env,
p->getter,
@ -1131,6 +1123,21 @@ napi_status napi_define_properties(napi_env env,
if (!set_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_invalid_arg);
}
} else if (p->method != nullptr) {
v8::Local<v8::Object> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
auto define_maybe =
obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
if (!define_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_generic_failure);
}
} else {
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);

8
src/node_api_types.h
View File

@ -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 {

16
test/addons-napi/test_constructor/test.js
View File

@ -17,7 +17,7 @@ assert.throws(() => { test_object.readonlyValue = 3; });
assert.ok(test_object.hiddenValue);
// All properties except 'hiddenValue' should be enumerable.
// Properties with napi_enumerable attribute should be enumerable.
const propertyNames = [];
for (const name in test_object) {
propertyNames.push(name);
@ -26,3 +26,17 @@ assert.ok(propertyNames.indexOf('echo') >= 0);
assert.ok(propertyNames.indexOf('readwriteValue') >= 0);
assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
assert.ok(propertyNames.indexOf('hiddenValue') < 0);
assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
assert.ok(propertyNames.indexOf('readonlyAccessor1') < 0);
assert.ok(propertyNames.indexOf('readonlyAccessor2') < 0);
// The napi_writable attribute should be ignored for accessors.
test_object.readwriteAccessor1 = 1;
assert.strictEqual(test_object.readwriteAccessor1, 1);
assert.strictEqual(test_object.readonlyAccessor1, 1);
assert.throws(() => { test_object.readonlyAccessor1 = 3; });
test_object.readwriteAccessor2 = 2;
assert.strictEqual(test_object.readwriteAccessor2, 2);
assert.strictEqual(test_object.readonlyAccessor2, 2);
assert.throws(() => { test_object.readonlyAccessor2 = 3; });

13
test/addons-napi/test_constructor/test_constructor.c
View File

@ -82,11 +82,14 @@ 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 },
{ "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},
{ "readwriteAccessor1", 0, GetValue, SetValue, 0, napi_default, 0},
{ "readwriteAccessor2", 0, GetValue, SetValue, 0, napi_writable, 0},
{ "readonlyAccessor1", 0, GetValue, NULL, 0, napi_default, 0},
{ "readonlyAccessor2", 0, GetValue, NULL, 0, napi_writable, 0},
};
napi_value cons;

16
test/addons-napi/test_properties/test.js
View File

@ -16,7 +16,7 @@ assert.throws(() => { test_object.readonlyValue = 3; });
assert.ok(test_object.hiddenValue);
// All properties except 'hiddenValue' should be enumerable.
// Properties with napi_enumerable attribute should be enumerable.
const propertyNames = [];
for (const name in test_object) {
propertyNames.push(name);
@ -25,3 +25,17 @@ assert.ok(propertyNames.indexOf('echo') >= 0);
assert.ok(propertyNames.indexOf('readwriteValue') >= 0);
assert.ok(propertyNames.indexOf('readonlyValue') >= 0);
assert.ok(propertyNames.indexOf('hiddenValue') < 0);
assert.ok(propertyNames.indexOf('readwriteAccessor1') < 0);
assert.ok(propertyNames.indexOf('readwriteAccessor2') < 0);
assert.ok(propertyNames.indexOf('readonlyAccessor1') < 0);
assert.ok(propertyNames.indexOf('readonlyAccessor2') < 0);
// The napi_writable attribute should be ignored for accessors.
test_object.readwriteAccessor1 = 1;
assert.strictEqual(test_object.readwriteAccessor1, 1);
assert.strictEqual(test_object.readonlyAccessor1, 1);
assert.throws(() => { test_object.readonlyAccessor1 = 3; });
test_object.readwriteAccessor2 = 2;
assert.strictEqual(test_object.readwriteAccessor2, 2);
assert.strictEqual(test_object.readonlyAccessor2, 2);
assert.throws(() => { test_object.readonlyAccessor2 = 3; });

13
test/addons-napi/test_properties/test_properties.c
View File

@ -70,11 +70,14 @@ 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 },
{ "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},
{ "readwriteAccessor1", 0, GetValue, SetValue, 0, napi_default, 0},
{ "readwriteAccessor2", 0, GetValue, SetValue, 0, napi_writable, 0},
{ "readonlyAccessor1", 0, GetValue, NULL, 0, napi_default, 0},
{ "readonlyAccessor2", 0, GetValue, NULL, 0, napi_writable, 0},
};
status = napi_define_properties(

Write Preview
Loading…
Cancel
Save