Skip to content

Commit

Permalink
n-api: define ECMAScript-compliant accessors on napi_define_class
Browse files Browse the repository at this point in the history
PR-URL: #27851
Fixes: #26551
Fixes: nodejs/node-addon-api#485
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
legendecas authored and targos committed Jun 18, 2019
1 parent b60287d commit 9c19c4b
Showing 4 changed files with 67 additions and 38 deletions.
61 changes: 28 additions & 33 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
@@ -79,12 +79,10 @@ inline static 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) {
// The napi_writable attribute is ignored for accessor descriptors, but
// V8 would throw `TypeError`s on assignment with nonexistence of a setter.
if ((descriptor->getter == nullptr && descriptor->setter == nullptr) &&
(descriptor->attributes & napi_writable) == 0) {
attribute_flags |= v8::PropertyAttribute::ReadOnly;
}

@@ -598,24 +596,6 @@ v8::Local<v8::Value> CreateFunctionCallbackData(napi_env env,
return cbdata;
}

// Creates an object to be made available to the static getter/setter
// callback wrapper, used to retrieve the native getter/setter callback
// function and data pointer.
inline v8::Local<v8::Value> CreateAccessorCallbackData(napi_env env,
napi_callback getter,
napi_callback setter,
void* data) {
CallbackBundle* bundle = new CallbackBundle();
bundle->function_or_getter = getter;
bundle->setter = setter;
bundle->cb_data = data;
bundle->env = env;
v8::Local<v8::Value> cbdata = v8::External::New(env->isolate, bundle);
bundle->BindLifecycleTo(env->isolate, cbdata);

return cbdata;
}

enum WrapType {
retrievable,
anonymous
@@ -812,18 +792,33 @@ napi_status napi_define_class(napi_env env,
v8impl::V8PropertyAttributesFromDescriptor(p);

// This code is similar to that in napi_define_properties(); the
// difference is it applies to a template instead of an object.
// difference is it applies to a template instead of an object,
// and preferred PropertyAttribute for lack of PropertyDescriptor
// support on ObjectTemplate.
if (p->getter != nullptr || p->setter != nullptr) {
v8::Local<v8::Value> cbdata = v8impl::CreateAccessorCallbackData(
env, p->getter, p->setter, p->data);
v8::Local<v8::FunctionTemplate> getter_tpl;
v8::Local<v8::FunctionTemplate> setter_tpl;
if (p->getter != nullptr) {
v8::Local<v8::Value> getter_data =
v8impl::CreateFunctionCallbackData(env, p->getter, p->data);

getter_tpl = v8::FunctionTemplate::New(
isolate, v8impl::FunctionCallbackWrapper::Invoke, getter_data);
}
if (p->setter != nullptr) {
v8::Local<v8::Value> setter_data =
v8impl::CreateFunctionCallbackData(env, p->setter, p->data);

setter_tpl = v8::FunctionTemplate::New(
isolate, v8impl::FunctionCallbackWrapper::Invoke, setter_data);
}

tpl->PrototypeTemplate()->SetAccessor(
tpl->PrototypeTemplate()->SetAccessorProperty(
property_name,
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
cbdata,
v8::AccessControl::DEFAULT,
attributes);
getter_tpl,
setter_tpl,
attributes,
v8::AccessControl::DEFAULT);
} else if (p->method != nullptr) {
v8::Local<v8::Value> cbdata =
v8impl::CreateFunctionCallbackData(env, p->method, p->data);
6 changes: 5 additions & 1 deletion test/js-native-api/6_object_wrap/myobject.cc
Original file line number Diff line number Diff line change
@@ -17,13 +17,17 @@ void MyObject::Destructor(
void MyObject::Init(napi_env env, napi_value exports) {
napi_property_descriptor properties[] = {
{ "value", nullptr, nullptr, GetValue, SetValue, 0, napi_default, 0 },
{ "valueReadonly", nullptr, nullptr, GetValue, nullptr, 0, napi_default,
0 },
DECLARE_NAPI_PROPERTY("plusOne", PlusOne),
DECLARE_NAPI_PROPERTY("multiply", Multiply),
};

napi_value cons;
NAPI_CALL_RETURN_VOID(env, napi_define_class(
env, "MyObject", -1, New, nullptr, 3, properties, &cons));
env, "MyObject", -1, New, nullptr,
sizeof(properties) / sizeof(napi_property_descriptor),
properties, &cons));

NAPI_CALL_RETURN_VOID(env, napi_create_reference(env, cons, 1, &constructor));

29 changes: 29 additions & 0 deletions test/js-native-api/6_object_wrap/test.js
Original file line number Diff line number Diff line change
@@ -3,10 +3,38 @@ const common = require('../../common');
const assert = require('assert');
const addon = require(`./build/${common.buildType}/binding`);

const getterOnlyErrorRE =
/^TypeError: Cannot set property .* of #<.*> which has only a getter$/;

const valueDescriptor = Object.getOwnPropertyDescriptor(
addon.MyObject.prototype, 'value');
const valueReadonlyDescriptor = Object.getOwnPropertyDescriptor(
addon.MyObject.prototype, 'valueReadonly');
const plusOneDescriptor = Object.getOwnPropertyDescriptor(
addon.MyObject.prototype, 'plusOne');
assert.strictEqual(typeof valueDescriptor.get, 'function');
assert.strictEqual(typeof valueDescriptor.set, 'function');
assert.strictEqual(valueDescriptor.value, undefined);
assert.strictEqual(valueDescriptor.enumerable, false);
assert.strictEqual(valueDescriptor.configurable, false);
assert.strictEqual(typeof valueReadonlyDescriptor.get, 'function');
assert.strictEqual(valueReadonlyDescriptor.set, undefined);
assert.strictEqual(valueReadonlyDescriptor.value, undefined);
assert.strictEqual(valueReadonlyDescriptor.enumerable, false);
assert.strictEqual(valueReadonlyDescriptor.configurable, false);

assert.strictEqual(plusOneDescriptor.get, undefined);
assert.strictEqual(plusOneDescriptor.set, undefined);
assert.strictEqual(typeof plusOneDescriptor.value, 'function');
assert.strictEqual(plusOneDescriptor.enumerable, false);
assert.strictEqual(plusOneDescriptor.configurable, false);

const obj = new addon.MyObject(9);
assert.strictEqual(obj.value, 9);
obj.value = 10;
assert.strictEqual(obj.value, 10);
assert.strictEqual(obj.valueReadonly, 10);
assert.throws(() => { obj.valueReadonly = 14; }, getterOnlyErrorRE);
assert.strictEqual(obj.plusOne(), 11);
assert.strictEqual(obj.plusOne(), 12);
assert.strictEqual(obj.plusOne(), 13);
@@ -16,4 +44,5 @@ assert.strictEqual(obj.multiply(10).value, 130);

const newobj = obj.multiply(-1);
assert.strictEqual(newobj.value, -13);
assert.strictEqual(newobj.valueReadonly, -13);
assert.notStrictEqual(obj, newobj);
9 changes: 5 additions & 4 deletions test/js-native-api/test_constructor/test.js
Original file line number Diff line number Diff line change
@@ -2,6 +2,9 @@
const common = require('../../common');
const assert = require('assert');

const getterOnlyErrorRE =
/^TypeError: Cannot set property .* of #<.*> which has only a getter$/;

// Testing api calls for a constructor that defines properties
const TestConstructor = require(`./build/${common.buildType}/test_constructor`);
const test_object = new TestConstructor();
@@ -36,13 +39,11 @@ assert.ok(!propertyNames.includes('readonlyAccessor2'));
test_object.readwriteAccessor1 = 1;
assert.strictEqual(test_object.readwriteAccessor1, 1);
assert.strictEqual(test_object.readonlyAccessor1, 1);
assert.throws(() => { test_object.readonlyAccessor1 = 3; },
/^TypeError: Cannot assign to read only property 'readonlyAccessor1' of object '#<MyObject>'$/);
assert.throws(() => { test_object.readonlyAccessor1 = 3; }, getterOnlyErrorRE);
test_object.readwriteAccessor2 = 2;
assert.strictEqual(test_object.readwriteAccessor2, 2);
assert.strictEqual(test_object.readonlyAccessor2, 2);
assert.throws(() => { test_object.readonlyAccessor2 = 3; },
/^TypeError: Cannot assign to read only property 'readonlyAccessor2' of object '#<MyObject>'$/);
assert.throws(() => { test_object.readonlyAccessor2 = 3; }, getterOnlyErrorRE);

// Validate that static properties are on the class as opposed
// to the instance

0 comments on commit 9c19c4b

Please sign in to comment.