n-api: add napi_has_own_property() #14063
Conversation
nodejs-github-bot
added
c++
doc/api/n-api.md
Outdated
|
|
||
| Returns `napi_ok` if the API succeeded. | ||
|
|
||
| This API checks if the Object passed in has the named own property. |
There was a problem hiding this comment.
I would suggest being explicit about Symbol support here
doc/api/n-api.md
Outdated
|
|
||
| Returns `napi_ok` if the API succeeded. | ||
|
|
||
| This API checks if the Object passed in has the named own property. |
There was a problem hiding this comment.
Can we add a note about side effects? E.g., what if the object is a revoked proxy? If the key is not to name?
| assert.strictEqual(test_object.HasOwn(obj, 'baz'), false); | ||
| assert.strictEqual(test_object.HasOwn(obj, 'toString'), false); | ||
| assert.strictEqual(test_object.HasOwn(obj, symbol2), false); | ||
| } |
There was a problem hiding this comment.
Can we add a test case where the key is not a name?
|
|
||
| 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
@fhinkel I added a note to the docs that no type conversions will be performed. That should cover ToPrimitive and any other implicit conversions.
|
Nits addressed, minus the |
|
|
||
| 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); |
There was a problem hiding this comment.
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.)
Refs: nodejs#13925 PR-URL: nodejs#14063 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Refs: #13925 PR-URL: #14063 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Refs: #13925 PR-URL: #14063 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Refs: #13925 PR-URL: #14063 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Refs: nodejs#13925 PR-URL: nodejs#14063 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Refs: #13925 Backport-PR-URL: #19447 PR-URL: #14063 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
Refs: #13925
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
n-api