Skip to content

Commit

Permalink
n-api: make napi_get_property_names return strings
Browse files Browse the repository at this point in the history
The documentation says that this method returns an array of strings.
Currently, it does not do so for indices. Resolve that by telling
V8 explicitly to convert to string.

PR-URL: #27524
Fixes: #27496
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
addaleax authored and targos committed May 6, 2019
1 parent 3d37414 commit e5fdc30
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,14 @@ napi_status napi_get_property_names(napi_env env,
v8::Local<v8::Object> obj;
CHECK_TO_OBJECT(env, context, obj, object);

auto maybe_propertynames = obj->GetPropertyNames(context);
v8::MaybeLocal<v8::Array> maybe_propertynames = obj->GetPropertyNames(
context,
v8::KeyCollectionMode::kIncludePrototypes,
static_cast<v8::PropertyFilter>(
v8::PropertyFilter::ONLY_ENUMERABLE |
v8::PropertyFilter::SKIP_SYMBOLS),
v8::IndexFilter::kIncludeIndices,
v8::KeyConversionMode::kConvertToString);

CHECK_MAYBE_EMPTY(env, maybe_propertynames, napi_generic_failure);

Expand Down
23 changes: 23 additions & 0 deletions test/js-native-api/test_object/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,26 @@ assert.strictEqual(newObject.test_string, 'test string');
assert.strictEqual(test_object.Delete(obj, 'foo'), true);
assert.strictEqual(obj.foo, 'baz');
}

{
// Verify that napi_get_property_names gets the right set of property names,
// i.e.: includes prototypes, only enumerable properties, skips symbols,
// and includes indices and converts them to strings.

const object = Object.create({
inherited: 1
});

object.normal = 2;
object[Symbol('foo')] = 3;
Object.defineProperty(object, 'unenumerable', {
value: 4,
enumerable: false,
writable: true,
configurable: true
});
object[5] = 5;

assert.deepStrictEqual(test_object.GetPropertyNames(object),
['5', 'normal', 'inherited']);
}
20 changes: 20 additions & 0 deletions test/js-native-api/test_object/test_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,25 @@ static napi_value GetNamed(napi_env env, napi_callback_info info) {
return output;
}

static napi_value GetPropertyNames(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value args[1];
NAPI_CALL(env, napi_get_cb_info(env, info, &argc, args, NULL, NULL));

NAPI_ASSERT(env, argc >= 1, "Wrong number of arguments");

napi_valuetype value_type0;
NAPI_CALL(env, napi_typeof(env, args[0], &value_type0));

NAPI_ASSERT(env, value_type0 == napi_object,
"Wrong type of arguments. Expects an object as first argument.");

napi_value output;
NAPI_CALL(env, napi_get_property_names(env, args[0], &output));

return output;
}

static napi_value Set(napi_env env, napi_callback_info info) {
size_t argc = 3;
napi_value args[3];
Expand Down Expand Up @@ -325,6 +344,7 @@ napi_value Init(napi_env env, napi_value exports) {
napi_property_descriptor descriptors[] = {
DECLARE_NAPI_PROPERTY("Get", Get),
DECLARE_NAPI_PROPERTY("GetNamed", GetNamed),
DECLARE_NAPI_PROPERTY("GetPropertyNames", GetPropertyNames),
DECLARE_NAPI_PROPERTY("Set", Set),
DECLARE_NAPI_PROPERTY("SetNamed", SetNamed),
DECLARE_NAPI_PROPERTY("Has", Has),
Expand Down

0 comments on commit e5fdc30

Please sign in to comment.