Skip to content

Commit

Permalink
n-api: define ECMAScript-compliant accessors on napi_define_properties
Browse files Browse the repository at this point in the history 
PR-URL: nodejs#27851
Fixes: nodejs#26551
Fixes: 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>
  • Loading branch information
@legendecas @BridgeAR
legendecas authored and BridgeAR committed Jun 17, 2019
1 parent 44f18d2 commit 4329585
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 22 deletions.
70 changes: 50 additions & 20 deletions src/js_native_api_v8.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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);

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();
}

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) {
Expand All @@ -1213,17 +1232,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);
Expand Down
16 changes: 14 additions & 2 deletions test/js-native-api/test_properties/test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
Expand Down Expand Up @@ -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'),
Expand Down

0 comments on commit 4329585

Please sign in to comment.