Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

n-api: add napi_has_own_property() #14063

Merged
merged 1 commit into from
Jul 6, 2017
Merged

n-api: add napi_has_own_property() #14063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions doc/api/n-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2298,6 +2298,29 @@ Returns `napi_ok` if the API succeeded.
This API attempts to delete the `key` own property from `object`.


#### *napi_has_own_property*
<!-- YAML
added: REPLACEME
-->
```C
napi_status napi_has_own_property(napi_env env,
napi_value object,
napi_value key,
bool* result);
```

- `[in] env`: The environment that the N-API call is invoked under.
- `[in] object`: The object to query.
- `[in] key`: The name of the own property whose existence to check.
- `[out] result`: Whether the own property exists on the object or not.

Returns `napi_ok` if the API succeeded.

This API checks if the Object passed in has the named own property. `key` must
be a string or a Symbol, or an error will be thrown. N-API will not perform any
conversion between data types.


#### *napi_set_named_property*
<!-- YAML
added: v8.0.0
Expand Down Expand Up @@ -3102,6 +3125,7 @@ support it:
[`napi_get_element`]: #n_api_napi_get_element
[`napi_get_property`]: #n_api_napi_get_property
[`napi_has_property`]: #n_api_napi_has_property
[`napi_has_own_property`]: #n_api_napi_has_own_property
[`napi_set_property`]: #n_api_napi_set_property
[`napi_get_reference_value`]: #n_api_napi_get_reference_value
[`napi_is_error`]: #n_api_napi_is_error
Expand Down
21 changes: 21 additions & 0 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,27 @@ napi_status napi_delete_property(napi_env env,
return GET_RETURN_STATUS(env);
}

NAPI_EXTERN napi_status napi_has_own_property(napi_env env,
napi_value object,
napi_value key,
bool* result) {
NAPI_PREAMBLE(env);
CHECK_ARG(env, key);

v8::Isolate* isolate = env->isolate;
v8::Local<v8::Context> context = isolate->GetCurrentContext();
v8::Local<v8::Object> obj;

CHECK_TO_OBJECT(env, context, obj, object);
v8::Local<v8::Value> k = v8impl::V8LocalValueFromJsValue(key);
RETURN_STATUS_IF_FALSE(env, k->IsName(), napi_name_expected);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than returning false, shouldn't we call the ToPrimitive method on the key? I'm thinking of an example like this:

var x = {hello: 17}
var o = { [Symbol.toPrimitive] : function() {return 'hello';}}
x.hasOwnProperty(o) // true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fhinkel what ToPrimitive() method? I'm not seeing anything exposed by the V8 API except for retrieving the symbol itself, but maybe I'm just missing it.

Also cc @nodejs/n-api as this case isn't handled anywhere else in n-api.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we expose it by itself. According to the spec, the key is converted to a name, which calls toPrimitive, toPropertyKey.

So, I think we should delete the return false if not IsName(), and let V8 handle it in a spec compliant way. V8 only needs a value, not a string/symbol here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for letting V8 handle this, but I get a compiler error passing in a Value :-/

../src/node_api.cc:1042:60: note: in instantiation of function template
      specialization 'v8::Local<v8::Name>::Local<v8::Value>' requested here
  v8::Maybe<bool> has_maybe = obj->HasOwnProperty(context, k);

It looks like V8 expects a Name.

Copy link
Member

@fhinkel fhinkel Jul 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I was looking at a comment for has(). Sorry for the confusion. I think we should make sure though that it's obvious that the n-api function is not identical to JS behavior, in particular, that we'll never use the ToPrimitive symbol if the key is an object. (We should probably point that out in the V8 API, but there it might be less of an issue because we only allow names.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fhinkel I added a note to the docs that no type conversions will be performed. That should cover ToPrimitive and any other implicit conversions.

v8::Maybe<bool> has_maybe = obj->HasOwnProperty(context, k.As<v8::Name>());
CHECK_MAYBE_NOTHING(env, has_maybe, napi_generic_failure);
*result = has_maybe.FromMaybe(false);

return GET_RETURN_STATUS(env);
}

napi_status napi_set_named_property(napi_env env,
napi_value object,
const char* utf8name,
Expand Down
4 changes: 4 additions & 0 deletions src/node_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ NAPI_EXTERN napi_status napi_delete_property(napi_env env,
napi_value object,
napi_value key,
bool* result);
NAPI_EXTERN napi_status napi_has_own_property(napi_env env,
napi_value object,
napi_value key,
bool* result);
NAPI_EXTERN napi_status napi_set_named_property(napi_env env,
napi_value object,
const char* utf8name,
Expand Down
34 changes: 34 additions & 0 deletions test/addons-napi/test_object/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,40 @@ assert.strictEqual(newObject.test_string, 'test string');
Object.prototype.toString);
}

{
// Verify that napi_has_own_property() fails if property is not a name.
[true, false, null, undefined, {}, [], 0, 1, () => {}].forEach((value) => {
assert.throws(() => {
test_object.HasOwn({}, value);
}, /^Error: A string or symbol was expected$/);
});
}

{
// Verify that napi_has_own_property() does not walk the prototype chain.
const symbol1 = Symbol();
const symbol2 = Symbol();

function MyObject() {
this.foo = 42;
this.bar = 43;
this[symbol1] = 44;
}

MyObject.prototype.bar = 45;
MyObject.prototype.baz = 46;
MyObject.prototype[symbol2] = 47;

const obj = new MyObject();

assert.strictEqual(test_object.HasOwn(obj, 'foo'), true);
assert.strictEqual(test_object.HasOwn(obj, 'bar'), true);
assert.strictEqual(test_object.HasOwn(obj, symbol1), true);
assert.strictEqual(test_object.HasOwn(obj, 'baz'), false);
assert.strictEqual(test_object.HasOwn(obj, 'toString'), false);
assert.strictEqual(test_object.HasOwn(obj, symbol2), false);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case where the key is not a name?


{
// test_object.Inflate increases all properties by 1
const cube = {
Expand Down
29 changes: 29 additions & 0 deletions test/addons-napi/test_object/test_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,34 @@ napi_value Has(napi_env env, napi_callback_info info) {
return ret;
}

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

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

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

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

// napi_valuetype valuetype1;
// NAPI_CALL(env, napi_typeof(env, args[1], &valuetype1));
//
// NAPI_ASSERT(env, valuetype1 == napi_string || valuetype1 == napi_symbol,
// "Wrong type of arguments. Expects a string or symbol as second.");

bool has_property;
NAPI_CALL(env, napi_has_own_property(env, args[0], args[1], &has_property));

napi_value ret;
NAPI_CALL(env, napi_get_boolean(env, has_property, &ret));

return ret;
}

napi_value Delete(napi_env env, napi_callback_info info) {
size_t argc = 2;
napi_value args[2];
Expand Down Expand Up @@ -196,6 +224,7 @@ void Init(napi_env env, napi_value exports, napi_value module, void* priv) {
DECLARE_NAPI_PROPERTY("Get", Get),
DECLARE_NAPI_PROPERTY("Set", Set),
DECLARE_NAPI_PROPERTY("Has", Has),
DECLARE_NAPI_PROPERTY("HasOwn", HasOwn),
DECLARE_NAPI_PROPERTY("Delete", Delete),
DECLARE_NAPI_PROPERTY("New", New),
DECLARE_NAPI_PROPERTY("Inflate", Inflate),
Expand Down