Skip to content

Commit 2dfecc4

Browse files
committed
n-api: define ECMAScript-compliant accessors on napi_define_class
Fixes: nodejs/node-addon-api#485
1 parent be84fae commit 2dfecc4

File tree

4 files changed

+67
-38
lines changed

4 files changed

+67
-38
lines changed

src/js_native_api_v8.cc

Lines changed: 28 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,10 @@ inline static v8::PropertyAttribute V8PropertyAttributesFromDescriptor(
7676
const napi_property_descriptor* descriptor) {
7777
unsigned int attribute_flags = v8::PropertyAttribute::None;
7878

79-
if (descriptor->getter != nullptr || descriptor->setter != nullptr) {
80-
// The napi_writable attribute is ignored for accessor descriptors, but
81-
// V8 requires the ReadOnly attribute to match nonexistence of a setter.
82-
attribute_flags |= (descriptor->setter == nullptr ?
83-
v8::PropertyAttribute::ReadOnly : v8::PropertyAttribute::None);
84-
} else if ((descriptor->attributes & napi_writable) == 0) {
79+
// The napi_writable attribute is ignored for accessor descriptors, but
80+
// V8 would throw `TypeError`s on assignment with nonexistence of a setter.
81+
if ((descriptor->getter == nullptr && descriptor->setter == nullptr) &&
82+
(descriptor->attributes & napi_writable) == 0) {
8583
attribute_flags |= v8::PropertyAttribute::ReadOnly;
8684
}
8785

@@ -595,24 +593,6 @@ v8::Local<v8::Value> CreateFunctionCallbackData(napi_env env,
595593
return cbdata;
596594
}
597595

598-
// Creates an object to be made available to the static getter/setter
599-
// callback wrapper, used to retrieve the native getter/setter callback
600-
// function and data pointer.
601-
inline v8::Local<v8::Value> CreateAccessorCallbackData(napi_env env,
602-
napi_callback getter,
603-
napi_callback setter,
604-
void* data) {
605-
CallbackBundle* bundle = new CallbackBundle();
606-
bundle->function_or_getter = getter;
607-
bundle->setter = setter;
608-
bundle->cb_data = data;
609-
bundle->env = env;
610-
v8::Local<v8::Value> cbdata = v8::External::New(env->isolate, bundle);
611-
bundle->BindLifecycleTo(env->isolate, cbdata);
612-
613-
return cbdata;
614-
}
615-
616596
enum WrapType {
617597
retrievable,
618598
anonymous
@@ -805,18 +785,33 @@ napi_status napi_define_class(napi_env env,
805785
v8impl::V8PropertyAttributesFromDescriptor(p);
806786

807787
// This code is similar to that in napi_define_properties(); the
808-
// difference is it applies to a template instead of an object.
788+
// difference is it applies to a template instead of an object,
789+
// and preferred PropertyAttribute for lack of PropertyDescriptor
790+
// support on ObjectTemplate.
809791
if (p->getter != nullptr || p->setter != nullptr) {
810-
v8::Local<v8::Value> cbdata = v8impl::CreateAccessorCallbackData(
811-
env, p->getter, p->setter, p->data);
792+
v8::Local<v8::FunctionTemplate> getter_tpl;
793+
v8::Local<v8::FunctionTemplate> setter_tpl;
794+
if (p->getter != nullptr) {
795+
v8::Local<v8::Value> getter_data =
796+
v8impl::CreateFunctionCallbackData(env, p->getter, p->data);
797+
798+
getter_tpl = v8::FunctionTemplate::New(
799+
isolate, v8impl::FunctionCallbackWrapper::Invoke, getter_data);
800+
}
801+
if (p->setter != nullptr) {
802+
v8::Local<v8::Value> setter_data =
803+
v8impl::CreateFunctionCallbackData(env, p->setter, p->data);
804+
805+
setter_tpl = v8::FunctionTemplate::New(
806+
isolate, v8impl::FunctionCallbackWrapper::Invoke, setter_data);
807+
}
812808

813-
tpl->PrototypeTemplate()->SetAccessor(
809+
tpl->PrototypeTemplate()->SetAccessorProperty(
814810
property_name,
815-
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
816-
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
817-
cbdata,
818-
v8::AccessControl::DEFAULT,
819-
attributes);
811+
getter_tpl,
812+
setter_tpl,
813+
attributes,
814+
v8::AccessControl::DEFAULT);
820815
} else if (p->method != nullptr) {
821816
v8::Local<v8::Value> cbdata =
822817
v8impl::CreateFunctionCallbackData(env, p->method, p->data);

test/js-native-api/6_object_wrap/myobject.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,17 @@ void MyObject::Destructor(
1717
void MyObject::Init(napi_env env, napi_value exports) {
1818
napi_property_descriptor properties[] = {
1919
{ "value", nullptr, nullptr, GetValue, SetValue, 0, napi_default, 0 },
20+
{ "valueReadonly", nullptr, nullptr, GetValue, nullptr, 0, napi_default,
21+
0 },
2022
DECLARE_NAPI_PROPERTY("plusOne", PlusOne),
2123
DECLARE_NAPI_PROPERTY("multiply", Multiply),
2224
};
2325

2426
napi_value cons;
2527
NAPI_CALL_RETURN_VOID(env, napi_define_class(
26-
env, "MyObject", -1, New, nullptr, 3, properties, &cons));
28+
env, "MyObject", -1, New, nullptr,
29+
sizeof(properties) / sizeof(napi_property_descriptor),
30+
properties, &cons));
2731

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

test/js-native-api/6_object_wrap/test.js

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,38 @@ const common = require('../../common');
33
const assert = require('assert');
44
const addon = require(`./build/${common.buildType}/binding`);
55

6+
const getterOnlyErrorRE =
7+
/^TypeError: Cannot set property .* of #<.*> which has only a getter$/;
8+
9+
const valueDescriptor = Object.getOwnPropertyDescriptor(
10+
addon.MyObject.prototype, 'value');
11+
const valueReadonlyDescriptor = Object.getOwnPropertyDescriptor(
12+
addon.MyObject.prototype, 'valueReadonly');
13+
const plusOneDescriptor = Object.getOwnPropertyDescriptor(
14+
addon.MyObject.prototype, 'plusOne');
15+
assert.strictEqual(typeof valueDescriptor.get, 'function');
16+
assert.strictEqual(typeof valueDescriptor.set, 'function');
17+
assert.strictEqual(valueDescriptor.value, undefined);
18+
assert.strictEqual(valueDescriptor.enumerable, false);
19+
assert.strictEqual(valueDescriptor.configurable, false);
20+
assert.strictEqual(typeof valueReadonlyDescriptor.get, 'function');
21+
assert.strictEqual(valueReadonlyDescriptor.set, undefined);
22+
assert.strictEqual(valueReadonlyDescriptor.value, undefined);
23+
assert.strictEqual(valueReadonlyDescriptor.enumerable, false);
24+
assert.strictEqual(valueReadonlyDescriptor.configurable, false);
25+
26+
assert.strictEqual(plusOneDescriptor.get, undefined);
27+
assert.strictEqual(plusOneDescriptor.set, undefined);
28+
assert.strictEqual(typeof plusOneDescriptor.value, 'function');
29+
assert.strictEqual(plusOneDescriptor.enumerable, false);
30+
assert.strictEqual(plusOneDescriptor.configurable, false);
31+
632
const obj = new addon.MyObject(9);
733
assert.strictEqual(obj.value, 9);
834
obj.value = 10;
935
assert.strictEqual(obj.value, 10);
36+
assert.strictEqual(obj.valueReadonly, 10);
37+
assert.throws(() => { obj.valueReadonly = 14; }, getterOnlyErrorRE);
1038
assert.strictEqual(obj.plusOne(), 11);
1139
assert.strictEqual(obj.plusOne(), 12);
1240
assert.strictEqual(obj.plusOne(), 13);
@@ -16,4 +44,5 @@ assert.strictEqual(obj.multiply(10).value, 130);
1644

1745
const newobj = obj.multiply(-1);
1846
assert.strictEqual(newobj.value, -13);
47+
assert.strictEqual(newobj.valueReadonly, -13);
1948
assert.notStrictEqual(obj, newobj);

test/js-native-api/test_constructor/test.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22
const common = require('../../common');
33
const assert = require('assert');
44

5+
const getterOnlyErrorRE =
6+
/^TypeError: Cannot set property .* of #<.*> which has only a getter$/;
7+
58
// Testing api calls for a constructor that defines properties
69
const TestConstructor = require(`./build/${common.buildType}/test_constructor`);
710
const test_object = new TestConstructor();
@@ -36,13 +39,11 @@ assert.ok(!propertyNames.includes('readonlyAccessor2'));
3639
test_object.readwriteAccessor1 = 1;
3740
assert.strictEqual(test_object.readwriteAccessor1, 1);
3841
assert.strictEqual(test_object.readonlyAccessor1, 1);
39-
assert.throws(() => { test_object.readonlyAccessor1 = 3; },
40-
/^TypeError: Cannot assign to read only property 'readonlyAccessor1' of object '#<MyObject>'$/);
42+
assert.throws(() => { test_object.readonlyAccessor1 = 3; }, getterOnlyErrorRE);
4143
test_object.readwriteAccessor2 = 2;
4244
assert.strictEqual(test_object.readwriteAccessor2, 2);
4345
assert.strictEqual(test_object.readonlyAccessor2, 2);
44-
assert.throws(() => { test_object.readonlyAccessor2 = 3; },
45-
/^TypeError: Cannot assign to read only property 'readonlyAccessor2' of object '#<MyObject>'$/);
46+
assert.throws(() => { test_object.readonlyAccessor2 = 3; }, getterOnlyErrorRE);
4647

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

0 commit comments

Comments
 (0)