From 432958528ea5a68d68afe0f0bc379abd3f35e8a0 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 24 May 2019 21:48:21 +0800 Subject: [PATCH] n-api: define ECMAScript-compliant accessors on napi_define_properties 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 | 70 +++++++++++++++------- test/js-native-api/test_properties/test.js | 16 ++++- 2 files changed, 64 insertions(+), 22 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 1131e3a6d1d213..b99f08bbea4995 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -1178,26 +1178,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 cbdata = v8impl::CreateAccessorCallbackData( - env, - p->getter, - p->setter, - p->data); + v8::Local local_getter; + v8::Local 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 getter_data = + v8impl::CreateFunctionCallbackData(env, p->getter, p->data); + CHECK_MAYBE_EMPTY(env, getter_data, napi_generic_failure); + + v8::MaybeLocal 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 setter_data = + v8impl::CreateFunctionCallbackData(env, p->setter, p->data); + CHECK_MAYBE_EMPTY(env, setter_data, napi_generic_failure); + + v8::MaybeLocal 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(); + } - if (!set_maybe.FromMaybe(false)) { + 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) { @@ -1213,8 +1232,14 @@ 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); @@ -1222,8 +1247,13 @@ napi_status napi_define_properties(napi_env env, } else { v8::Local 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); diff --git a/test/js-native-api/test_properties/test.js b/test/js-native-api/test_properties/test.js index fa4cb587d6435a..704002dfc74f3a 100644 --- a/test/js-native-api/test_properties/test.js +++ b/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 '#'$/; +const getterOnlyErrorRE = + /^TypeError: Cannot set property .* of # 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'),