From 10a346edde47c872de9afbcb79aa43095064078b Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 29 May 2019 13:25:07 +0800 Subject: [PATCH] n-api: define ECMAScript-compliant accessors on napi_define_class PR-URL: https://github.com/nodejs/node/pull/27851 Fixes: https://github.com/nodejs/node/issues/26551 Fixes: https://github.com/nodejs/node-addon-api/issues/485 Reviewed-By: Anna Henningsen Reviewed-By: Gus Caplan Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: Michael Dawson --- src/js_native_api_v8.cc | 61 +++++++++----------- test/js-native-api/6_object_wrap/myobject.cc | 6 +- test/js-native-api/6_object_wrap/test.js | 29 ++++++++++ test/js-native-api/test_constructor/test.js | 9 +-- 4 files changed, 67 insertions(+), 38 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index b99f08bbea4995..30f34b474c2822 100644 --- 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 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 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 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 cbdata = v8impl::CreateAccessorCallbackData( - env, p->getter, p->setter, p->data); + v8::Local getter_tpl; + v8::Local setter_tpl; + if (p->getter != nullptr) { + v8::Local 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 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 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 index 4d3e16ca121382..30e6b66dcbe4b7 100644 --- 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 index 4d89da6a4350bd..f3535969c60fea 100644 --- 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 index 8dc987a99458cf..a58eac81ad5176 100644 --- 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 '#'$/); +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 '#'$/); +assert.throws(() => { test_object.readonlyAccessor2 = 3; }, getterOnlyErrorRE); // Validate that static properties are on the class as opposed // to the instance