Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 83fc10d

Browse filesBrowse files
legendecasBridgeAR
legendecas
authored and
BridgeAR
committed
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
Copy full SHA for 83fc10d

File tree

Expand file treeCollapse file tree

4 files changed

+67
-38
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+67
-38
lines changed
Open diff view settings
Collapse file

‎src/js_native_api_v8.cc‎

Copy file name to clipboardExpand all lines: src/js_native_api_v8.cc
+28-33Lines 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);
Collapse file

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

Copy file name to clipboardExpand all lines: test/js-native-api/6_object_wrap/myobject.cc
+5-1Lines 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

Collapse file

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

Copy file name to clipboardExpand all lines: test/js-native-api/6_object_wrap/test.js
+29Lines 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);
Collapse file

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

Copy file name to clipboardExpand all lines: test/js-native-api/test_constructor/test.js
+5-4Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.