Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api: Update property attrs enum to match JS spec #12240

Closed
wants to merge 3 commits into from
Closed

n-api: Update property attrs enum to match JS spec #12240

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44cef98
n-api: Update property attrs enum to match JS spec
jasongin Apr 5, 2017
05586f4
Refactor attribute normalization, add tests
jasongin Apr 6, 2017
d915a24
Small updates for review feedback
jasongin Apr 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 59 additions & 51 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/******************************************************************************
/******************************************************************************
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an idea what this is?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a UTF-8 BOM. VS 2017 likes to insert those for some reason. Fixed.

* Experimental prototype for demonstrating VM agnostic and ABI stable API
* for native modules to use instead of using Nan and V8 APIs directly.
*
Expand Down Expand Up @@ -35,18 +35,27 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: coalesce the right brace with else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

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);
}

Expand Down Expand Up @@ -775,7 +784,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 @@ -784,25 +793,11 @@ 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) {
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,
v8::Signature::New(isolate, tpl));
t->SetClassName(property_name);

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

Expand All @@ -813,6 +808,20 @@ napi_status napi_define_class(napi_env env,
cbdata,
v8::AccessControl::DEFAULT,
attributes);
} 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,
v8::Signature::New(isolate, tpl));
t->SetClassName(property_name);

tpl->PrototypeTemplate()->Set(property_name, t, attributes);
} else {
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
tpl->PrototypeTemplate()->Set(property_name, value, attributes);
Expand All @@ -827,7 +836,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 +1103,28 @@ 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::V8PropertyAttributesFromDescriptor(p);

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

auto set_maybe = obj->SetAccessor(
context,
name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
cbdata,
v8::AccessControl::DEFAULT,
attributes);

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);

Expand All @@ -1112,25 +1139,6 @@ 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) {
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
env,
p->getter,
p->setter,
p->data);

auto set_maybe = obj->SetAccessor(
context,
name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
cbdata,
v8::AccessControl::DEFAULT,
attributes);

if (!set_maybe.FromMaybe(false)) {
return napi_set_last_error(env, napi_invalid_arg);
}
} else {
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);

Expand Down
8 changes: 4 additions & 4 deletions src/node_api_types.h
View file
Open in desktop
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
16 changes: 15 additions & 1 deletion test/addons-napi/test_constructor/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 changes: 8 additions & 5 deletions test/addons-napi/test_constructor/test_constructor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 15 additions & 1 deletion test/addons-napi/test_properties/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 changes: 8 additions & 5 deletions test/addons-napi/test_properties/test_properties.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down