n-api: add napi_delete_property() #13934
Conversation
nodejs-github-bot
added
c++
|
@mhdawson I changed the function signature slightly. I added a return value because |
src/node_api.cc
Outdated
| 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); | ||
| *result = delete_maybe.FromMaybe(false); |
There was a problem hiding this comment.
Check that result is not null before assigning to it. Some callers may not care about the result, so allow them to pass null to ignore it.
src/node_api.cc
Outdated
| napi_value key, | ||
| bool* result) { | ||
| NAPI_PREAMBLE(env); | ||
| CHECK_ENV(env); |
There was a problem hiding this comment.
NAPI_PREAMBLE(env) already calls CHECK_ENV(env), so the latter is redundant here.
doc/api/n-api.md
Outdated
| - `[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. | ||
| - `[out] result`: Whether the property deletion succeeded or not. |
|
Comments addressed. |
|
|
||
| - `[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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const sym = Symbol(); | ||
| const obj = { foo: 'bar', [sym]: 'baz' }; | ||
|
|
||
| assert.strictEqual('foo' in obj, true); |
There was a problem hiding this comment.
why not just assert('foo' in obj)?
There was a problem hiding this comment.
No specific reason. Just being more explicit.
Fixes: nodejs#13924 PR-URL: nodejs#13934 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Fixes: nodejs#13924 PR-URL: nodejs#13934 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Fixes: #13924 PR-URL: #13934 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Fixes: #13924 PR-URL: #13934 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Fixes: nodejs#13924 PR-URL: nodejs#13934 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Fixes: #13924 Backport-PR-URL: #19447 PR-URL: #13934 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>
Refs: #13924
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
n-api