Skip to content

Commit 83fc10d

Browse files
legendecasBridgeAR
authored andcommitted
n-api: define ECMAScript-compliant accessors on napi_define_class
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>
1 parent d70fd7d commit 83fc10d

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
@@ -79,12 +79,10 @@ inline static v8::PropertyAttribute V8PropertyAttributesFromDescriptor(
7979
const napi_property_descriptor* descriptor) {
8080
unsigned int attribute_flags = v8::PropertyAttribute::None;
8181

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

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

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

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

820-
tpl->PrototypeTemplate()->SetAccessor(
816+
tpl->PrototypeTemplate()->SetAccessorProperty(
821817
property_name,
822-
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
823-
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
824-
cbdata,
825-
v8::AccessControl::DEFAULT,
826-
attributes);
818+
getter_tpl,
819+
setter_tpl,
820+
attributes,
821+
v8::AccessControl::DEFAULT);
827822
} else if (p->method != nullptr) {
828823
v8::Local<v8::Value> cbdata =
829824
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)