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.

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

n-api: add napi_delete_property() #13934

Merged
merged 1 commit into from
Jul 1, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions doc/api/n-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -2276,6 +2276,28 @@ Returns `napi_ok` if the API succeeded.
This API checks if the Object passed in has the named property.


#### *napi_delete_property*
<!-- YAML
added: REPLACEME
-->
```C
napi_status napi_delete_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 property to delete.
Copy link
Member

Choose a reason for hiding this comment

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

Can key be a napi number? Either way it's fine but just thought that it could be helpful to point that out in the docs.

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 found this section in the documentation that provides some blanket statements for all of the methods for working with properties. Specifically, see:

JavaScript value: these are represented in N-API by napi_value. This can be a napi_value representing a String, Number or Symbol.

Given that, and the surrounding documentation, I'd prefer to keep this as is in this PR. However, I think the n-api documentation as a whole should be made more consistent with the rest of the Node docs. For example, I think each method should document that key can be a number, string, or symbol. That way, users don't have to read the entire page/section/whatever to get that information.

- `[out] result`: Whether the property deletion succeeded or not. `result` can
optionally be ignored by passing `NULL`.

Returns `napi_ok` if the API succeeded.

This API attempts to delete the `key` own property from `object`.


#### *napi_set_named_property*
<!-- YAML
added: v8.0.0
Expand Down Expand Up @@ -3073,6 +3095,7 @@ support it:
[`napi_delete_async_work`]: #n_api_napi_delete_async_work
[`napi_define_class`]: #n_api_napi_define_class
[`napi_delete_element`]: #n_api_napi_delete_element
[`napi_delete_property`]: #n_api_napi_delete_property
[`napi_delete_reference`]: #n_api_napi_delete_reference
[`napi_escape_handle`]: #n_api_napi_escape_handle
[`napi_get_array_length`]: #n_api_napi_get_array_length
Expand Down
22 changes: 22 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1002,6 +1002,28 @@ napi_status napi_get_property(napi_env env,
return GET_RETURN_STATUS(env);
}

napi_status napi_delete_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::Value> k = v8impl::V8LocalValueFromJsValue(key);
v8::Local<v8::Object> obj;

CHECK_TO_OBJECT(env, context, obj, object);
v8::Maybe<bool> delete_maybe = obj->Delete(context, k);
CHECK_MAYBE_NOTHING(env, delete_maybe, napi_generic_failure);

if (result != NULL)
*result = delete_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
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ NAPI_EXTERN napi_status napi_get_property(napi_env env,
napi_value object,
napi_value key,
napi_value* result);
NAPI_EXTERN napi_status napi_delete_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
44 changes: 44 additions & 0 deletions test/addons-napi/test_object/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,47 @@ assert.strictEqual(newObject.test_string, 'test string');
assert(wrapper.protoA, true);
assert(wrapper.protoB, true);
}

{
// Verify that normal and nonexistent properties can be deleted.
const sym = Symbol();
const obj = { foo: 'bar', [sym]: 'baz' };

assert.strictEqual('foo' in obj, true);
Copy link
Member

Choose a reason for hiding this comment

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

why not just assert('foo' in obj)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason. Just being more explicit.

assert.strictEqual(sym in obj, true);
assert.strictEqual('does_not_exist' in obj, false);
assert.strictEqual(test_object.Delete(obj, 'foo'), true);
assert.strictEqual('foo' in obj, false);
assert.strictEqual(sym in obj, true);
assert.strictEqual('does_not_exist' in obj, false);
assert.strictEqual(test_object.Delete(obj, sym), true);
assert.strictEqual('foo' in obj, false);
assert.strictEqual(sym in obj, false);
assert.strictEqual('does_not_exist' in obj, false);
}

{
// Verify that non-configurable properties are not deleted.
const obj = {};

Object.defineProperty(obj, 'foo', { configurable: false });
assert.strictEqual(test_object.Delete(obj, 'foo'), false);
assert.strictEqual('foo' in obj, true);
}

{
// Verify that prototype properties are not deleted.
function Foo() {
this.foo = 'bar';
}

Foo.prototype.foo = 'baz';

const obj = new Foo();

assert.strictEqual(obj.foo, 'bar');
assert.strictEqual(test_object.Delete(obj, 'foo'), true);
assert.strictEqual(obj.foo, 'baz');
assert.strictEqual(test_object.Delete(obj, 'foo'), true);
assert.strictEqual(obj.foo, 'baz');
}
26 changes: 26 additions & 0 deletions test/addons-napi/test_object/test_object.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,31 @@ napi_value Has(napi_env env, napi_callback_info info) {
return ret;
}

napi_value Delete(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 result;
napi_value ret;
NAPI_CALL(env, napi_delete_property(env, args[0], args[1], &result));
NAPI_CALL(env, napi_get_boolean(env, result, &ret));

return ret;
}

napi_value New(napi_env env, napi_callback_info info) {
napi_value ret;
NAPI_CALL(env, napi_create_object(env, &ret));
Expand Down Expand Up @@ -171,6 +196,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("Delete", Delete),
DECLARE_NAPI_PROPERTY("New", New),
DECLARE_NAPI_PROPERTY("Inflate", Inflate),
DECLARE_NAPI_PROPERTY("Wrap", Wrap),
Expand Down