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

Add API for deletion of object properties #151

Closed
wants to merge 3 commits into from
Closed
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
31 changes: 30 additions & 1 deletion napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,28 @@ inline void Object::Set(const std::string& utf8name, double numberValue) {
Set(utf8name.c_str(), Number::New(Env(), numberValue));
}

inline bool Object::Delete(napi_value key) {
bool result;
napi_status status = napi_delete_property(_env, _value, key, &result);
NAPI_THROW_IF_FAILED(_env, status, false);
return result;
}

inline bool Object::Delete(Value key) {
bool result;
napi_status status = napi_delete_property(_env, _value, key, &result);
NAPI_THROW_IF_FAILED(_env, status, false);
return result;
}

inline bool Object::Delete(const char* utf8name) {
return Delete(String::New(_env, utf8name));
}

inline bool Object::Delete(const std::string& utf8name) {
return Delete(String::New(_env, utf8name));
}

inline bool Object::Has(uint32_t index) const {
bool result;
napi_status status = napi_has_element(_env, _value, index, &result);
Expand Down Expand Up @@ -797,6 +819,13 @@ inline void Object::Set(uint32_t index, double numberValue) {
Set(index, static_cast<napi_value>(Number::New(Env(), numberValue)));
}

inline bool Object::Delete(uint32_t index) {
bool result;
napi_status status = napi_delete_element(_env, _value, index, &result);
NAPI_THROW_IF_FAILED(_env, status, false);
return result;
}

inline Array Object::GetPropertyNames() {
napi_value result;
napi_status status = napi_get_property_names(_env, _value, &result);
Expand Down Expand Up @@ -1657,7 +1686,7 @@ inline Reference<T>& Reference<T>::operator =(Reference<T>&& other) {
}

template <typename T>
inline Reference<T>::Reference(const Reference<T>& other)
inline Reference<T>::Reference(const Reference<T>& other)
: _env(other._env), _ref(nullptr), _suppressDestruct(false) {
HandleScope scope(_env);

Expand Down
25 changes: 25 additions & 0 deletions napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,26 @@ namespace Napi {
double numberValue ///< Property value
);

/// Delete property.
bool Delete(
napi_value key ///< Property key primitive
);

/// Delete property.
bool Delete(
Value key ///< Property key
);
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 need this key since Value has an operator napi_value member, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I guess so; I just copied the definitions from Object::Has. Should I remove all of them?

Copy link
Member

Choose a reason for hiding this comment

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

I think I had to add the overloads that take Value in order to avoid compiler errors in some situations about ambiguous calls to overloaded methods.

Copy link
Member

Choose a reason for hiding this comment

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

@jasongin I think it should be safe to remove those – if you’re talking about #86, that’s happening because there were two multiple arguments that were used for overloading …

@rolftimmermans I’m not sure, but it certainly doesn’t have to happen in this PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait, maybe that is about Strings that also have an operator std::string? That would make sense but we should have tests for that :)


/// Delete property.
bool Delete(
const char* utf8name ///< UTF-8 encoded null-terminated property name
);

/// Delete property.
bool Delete(
const std::string& utf8name ///< UTF-8 encoded property name
);

/// Checks whether an indexed property is present.
bool Has(
uint32_t index ///< Property / element index
Expand Down Expand Up @@ -537,6 +557,11 @@ namespace Napi {
double numberValue ///< Property value
);

/// Deletes an indexed property or array element.
bool Delete(
uint32_t index ///< Property / element index
);

Array GetPropertyNames(); ///< Get all property names

/// Defines a property on the object.
Expand Down
7 changes: 7 additions & 0 deletions test/object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ void SetProperty(const CallbackInfo& info) {
obj.Set(name, value);
}

Value DeleteProperty(const CallbackInfo& info) {
Object obj = info[0].As<Object>();
Name name = info[1].As<Name>();
return Boolean::New(info.Env(), obj.Delete(name));
}

Object InitObject(Env env) {
Object exports = Object::New(env);

Expand All @@ -110,6 +116,7 @@ Object InitObject(Env env) {
exports["defineValueProperty"] = Function::New(env, DefineValueProperty);
exports["getProperty"] = Function::New(env, GetProperty);
exports["setProperty"] = Function::New(env, SetProperty);
exports["deleteProperty"] = Function::New(env, DeleteProperty);

return exports;
}
15 changes: 15 additions & 0 deletions test/object.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,18 @@ function test(binding) {
assert.strictEqual(obj.test, 1);
}

{
const obj = { one: 1, two: 2 };
Object.defineProperty(obj, "three", {configurable: false, value: 3});
assert.strictEqual(binding.object.deleteProperty(obj, 'one'), true);
assert.strictEqual(binding.object.deleteProperty(obj, 'missing'), true);

/* Returns true for all cases except when the property is an own non-
configurable property, in which case, false is returned in non-strict mode. */
assert.strictEqual(binding.object.deleteProperty(obj, 'three'), false);
assert.deepStrictEqual(obj, { two: 2 });
}

{
const obj = {'one': 1, 'two': 2, 'three': 3};
var arr = binding.object.GetPropertyNames(obj);
Expand All @@ -94,4 +106,7 @@ function test(binding) {
assert.throws(() => {
binding.object.setProperty(undefined, 'test', 1);
}, /object was expected/);
assert.throws(() => {
binding.object.deleteProperty(undefined, 'test');
}, /object was expected/);
}