n-api: make napi_get_property_names return strings#27524
n-api: make napi_get_property_names return strings#27524addaleax wants to merge 1 commit intonodejs:masterfrom
Conversation
The documentation says that this method returns an array of strings. Currently, it does not do so for indices. Resolve that by telling V8 explicitly to convert to string. Fixes: nodejs#27496
nodejs-github-bot
added
the
c++
| auto maybe_propertynames = obj->GetPropertyNames(context); | ||
| v8::MaybeLocal<v8::Array> maybe_propertynames = obj->GetPropertyNames( | ||
| context, | ||
| v8::KeyCollectionMode::kIncludePrototypes, |
There was a problem hiding this comment.
Should napi_get_property_names return all property names including these on prototypes? Or kOwnOnly is just fine.
There was a problem hiding this comment.
I think that would have been a good idea when this API was originally conceived, but it would be a breaking change to switch this over, and we generally don’t do that for N-API.
There was a problem hiding this comment.
I think it's a good idea to add another napi_get_own_propery_names. I can do that if others agree.
There was a problem hiding this comment.
@BridgeAR I think that’s a good idea. I think we’d also maybe want to include non-enumerable properties/symbols, because there doesn’t currently appear to be a way to get those (maybe as an option, similar to the way in which the V8 API leaves us the choice).
| auto maybe_propertynames = obj->GetPropertyNames(context); | ||
| v8::MaybeLocal<v8::Array> maybe_propertynames = obj->GetPropertyNames( | ||
| context, | ||
| v8::KeyCollectionMode::kIncludePrototypes, |
There was a problem hiding this comment.
I think it's a good idea to add another napi_get_own_propery_names. I can do that if others agree.
BridgeAR
added
the
author ready
|
Landed in 5c08481. |
The documentation says that this method returns an array of strings. Currently, it does not do so for indices. Resolve that by telling V8 explicitly to convert to string. PR-URL: #27524 Fixes: #27496 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The documentation says that this method returns an array of strings. Currently, it does not do so for indices. Resolve that by telling V8 explicitly to convert to string. PR-URL: #27524 Fixes: #27496 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
7 participants
The documentation says that this method returns an array of strings.
Currently, it does not do so for indices. Resolve that by telling
V8 explicitly to convert to string.
Fixes: #27496
/cc @nodejs/n-api
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes