n-api: define ECMAScript-compliant accessors on napi_define_class

PR-URL: #27851Fixes: #26551Fixes: 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>

Author: legendecas

src/js_native_api_v8.cc

@@ -79,12 +79,10 @@ inline static v8::PropertyAttribute V8PropertyAttributesFromDescriptor(
 const napi_property_descriptor* descriptor) {
 unsignedint 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);
- } elseif ((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 = newCallbackBundle();
- 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);
  } elseif (p->method != nullptr) {
  v8::Local<v8::Value> cbdata =
 v8impl::CreateFunctionCallbackData(env, p->method, p->data);

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

@@ -17,13 +17,17 @@ void MyObject::Destructor(
 voidMyObject::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));
 

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

@@ -3,10 +3,38 @@ const common = require('../../common');
 constassert=require('assert');
 constaddon=require(`./build/${common.buildType}/binding`);
 
+constgetterOnlyErrorRE=
+/^TypeError:Cannotsetproperty.*of#<.*>whichhasonlyagetter$/;
+
+constvalueDescriptor=Object.getOwnPropertyDescriptor(
+addon.MyObject.prototype,'value');
+constvalueReadonlyDescriptor=Object.getOwnPropertyDescriptor(
+addon.MyObject.prototype,'valueReadonly');
+constplusOneDescriptor=Object.getOwnPropertyDescriptor(
+addon.MyObject.prototype,'plusOne');
+assert.strictEqual(typeofvalueDescriptor.get,'function');
+assert.strictEqual(typeofvalueDescriptor.set,'function');
+assert.strictEqual(valueDescriptor.value,undefined);
+assert.strictEqual(valueDescriptor.enumerable,false);
+assert.strictEqual(valueDescriptor.configurable,false);
+assert.strictEqual(typeofvalueReadonlyDescriptor.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(typeofplusOneDescriptor.value,'function');
+assert.strictEqual(plusOneDescriptor.enumerable,false);
+assert.strictEqual(plusOneDescriptor.configurable,false);
+
 constobj=newaddon.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);
 
 constnewobj=obj.multiply(-1);
 assert.strictEqual(newobj.value,-13);
+assert.strictEqual(newobj.valueReadonly,-13);
 assert.notStrictEqual(obj,newobj);

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

@@ -2,6 +2,9 @@
 constcommon=require('../../common');
 constassert=require('assert');
 
+constgetterOnlyErrorRE=
+/^TypeError:Cannotsetproperty.*of#<.*>whichhasonlyagetter$/;
+
 // Testing api calls for a constructor that defines properties
 constTestConstructor=require(`./build/${common.buildType}/test_constructor`);
 consttest_object=newTestConstructor();
@@ -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:Cannotassigntoreadonlyproperty'readonlyAccessor1'ofobject'#<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:Cannotassigntoreadonlyproperty'readonlyAccessor2'ofobject'#<MyObject>'$/);
+assert.throws(()=>{test_object.readonlyAccessor2=3;},getterOnlyErrorRE);
 
 // Validate that static properties are on the class as opposed
 // to the instance