n-api: define ECMAScript-compliant accessors on napi_define_*

src/js_native_api_v8.cc

@@ -76,12 +76,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;
   }
 
@@ -595,24 +593,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
@@ -805,18 +785,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);
 
-      tpl->PrototypeTemplate()->SetAccessor(
+        setter_tpl = v8::FunctionTemplate::New(
+            isolate, v8impl::FunctionCallbackWrapper::Invoke, setter_data);
+      }
+
+      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);
@@ -1171,26 +1166,45 @@ napi_status napi_define_properties(napi_env env,
       return napi_set_last_error(env, status);
     }
 
-    v8::PropertyAttribute attributes =
-        v8impl::V8PropertyAttributesFromDescriptor(p);
-
     if (p->getter != nullptr || p->setter != nullptr) {
-      v8::Local<v8::Value> cbdata = v8impl::CreateAccessorCallbackData(
-        env,
-        p->getter,
-        p->setter,
-        p->data);
+      v8::Local<v8::Value> local_getter;
+      v8::Local<v8::Value> local_setter;
 
-      auto set_maybe = obj->SetAccessor(
-        context,
-        property_name,
-        p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
-        p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
-        cbdata,
-        v8::AccessControl::DEFAULT,
-        attributes);
+      if (p->getter != nullptr) {
+        v8::Local<v8::Value> getter_data =
+            v8impl::CreateFunctionCallbackData(env, p->getter, p->data);
+        CHECK_MAYBE_EMPTY(env, getter_data, napi_generic_failure);
 
-      if (!set_maybe.FromMaybe(false)) {
+        v8::MaybeLocal<v8::Function> maybe_getter =
+            v8::Function::New(context,
+                              v8impl::FunctionCallbackWrapper::Invoke,
+                              getter_data);
+        CHECK_MAYBE_EMPTY(env, maybe_getter, napi_generic_failure);
+
+        local_getter = maybe_getter.ToLocalChecked();
+      }
+      if (p->setter != nullptr) {
+        v8::Local<v8::Value> setter_data =
+            v8impl::CreateFunctionCallbackData(env, p->setter, p->data);
+        CHECK_MAYBE_EMPTY(env, setter_data, napi_generic_failure);
+
+        v8::MaybeLocal<v8::Function> maybe_setter =
+            v8::Function::New(context,
+                              v8impl::FunctionCallbackWrapper::Invoke,
+                              setter_data);
+        CHECK_MAYBE_EMPTY(env, maybe_setter, napi_generic_failure);
+        local_setter = maybe_setter.ToLocalChecked();
+      }
+
+      v8::PropertyDescriptor descriptor(local_getter, local_setter);
+      descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
+      descriptor.set_configurable((p->attributes & napi_configurable) != 0);
+
+      auto define_maybe = obj->DefineProperty(context,
+                                              property_name,
+                                              descriptor);
+
+      if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_invalid_arg);
       }
     } else if (p->method != nullptr) {
@@ -1206,17 +1220,28 @@ napi_status napi_define_properties(napi_env env,
 
       CHECK_MAYBE_EMPTY(env, maybe_fn, napi_generic_failure);
 
-      auto define_maybe = obj->DefineOwnProperty(
-        context, property_name, maybe_fn.ToLocalChecked(), attributes);
+      v8::PropertyDescriptor descriptor(maybe_fn.ToLocalChecked(),
+                                        (p->attributes & napi_writable) != 0);
+      descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
+      descriptor.set_configurable((p->attributes & napi_configurable) != 0);
+
+      auto define_maybe = obj->DefineProperty(context,
+                                              property_name,
+                                              descriptor);
 
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_generic_failure);
       }
     } else {
       v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
 
+      v8::PropertyDescriptor descriptor(value,
+                                        (p->attributes & napi_writable) != 0);
+      descriptor.set_enumerable((p->attributes & napi_enumerable) != 0);
+      descriptor.set_configurable((p->attributes & napi_configurable) != 0);
+
       auto define_maybe =
-          obj->DefineOwnProperty(context, property_name, value, attributes);
+          obj->DefineProperty(context, property_name, descriptor);
 
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_invalid_arg);

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

@@ -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));
 

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

@@ -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);

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

@@ -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

test/js-native-api/test_properties/test.js

@@ -3,6 +3,8 @@ const common = require('../../common');
 const assert = require('assert');
 const readonlyErrorRE =
   /^TypeError: Cannot assign to read only property '.*' of object '#<Object>'$/;
+const getterOnlyErrorRE =
+  /^TypeError: Cannot set property .* of #<Object> which has only a getter$/;
 
 // Testing api calls for defining properties
 const test_object = require(`./build/${common.buildType}/test_properties`);
@@ -41,14 +43,24 @@ const symbolDescription =
 assert.strictEqual(symbolDescription, 'NameKeySymbol');
 
 // The napi_writable attribute should be ignored for accessors.
+const readwriteAccessor1Descriptor =
+  Object.getOwnPropertyDescriptor(test_object, 'readwriteAccessor1');
+const readonlyAccessor1Descriptor =
+  Object.getOwnPropertyDescriptor(test_object, 'readonlyAccessor1');
+assert.ok(readwriteAccessor1Descriptor.get != null);
+assert.ok(readwriteAccessor1Descriptor.set != null);
+assert.ok(readwriteAccessor1Descriptor.value === undefined);
+assert.ok(readonlyAccessor1Descriptor.get != null);
+assert.ok(readonlyAccessor1Descriptor.set === undefined);
+assert.ok(readonlyAccessor1Descriptor.value === undefined);
 test_object.readwriteAccessor1 = 1;
 assert.strictEqual(test_object.readwriteAccessor1, 1);
 assert.strictEqual(test_object.readonlyAccessor1, 1);
-assert.throws(() => { test_object.readonlyAccessor1 = 3; }, readonlyErrorRE);
+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; }, readonlyErrorRE);
+assert.throws(() => { test_object.readonlyAccessor2 = 3; }, getterOnlyErrorRE);
 
 assert.strictEqual(test_object.hasNamedProperty(test_object, 'echo'), true);
 assert.strictEqual(test_object.hasNamedProperty(test_object, 'hiddenValue'),