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 <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
nodejs · Jun 17, 2019 · 83fc10d · 83fc10d
1 parent d70fd7d
commit 83fc10d
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 38 deletions.
diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc
@@ -79,12 +79,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;
   }
 
@@ -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 = 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
@@ -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);
     } else if (p->method != nullptr) {
       v8::Local<v8::Value> cbdata =
           v8impl::CreateFunctionCallbackData(env, p->method, p->data);

diff --git a/test/js-native-api/6_object_wrap/myobject.cc b/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));
 

diff --git a/test/js-native-api/6_object_wrap/test.js b/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);
diff --git a/test/js-native-api/test_constructor/test.js b/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