feat: add Object::New overload utilizing node_api_create_object_with_properties - #1735
Conversation
bbf03d9 to
ef2601b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
- Coverage 63.69% 63.68% -0.02%
==========================================
Files 3 3
Lines 2063 2065 +2
Branches 730 731 +1
==========================================
+ Hits 1314 1315 +1
Misses 162 162
- Partials 587 588 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The CI failure is nodejs/node-gyp#3327 |
eb9b7ef to
e725910
Compare
e725910 to
23b62a9
Compare
| std::vector<napi_value>& propertyNames, | ||
| std::vector<napi_value>& propertyValues) { | ||
| if (propertyNames.size() != propertyValues.size()) { | ||
| Napi::Error::New(env, "Mismatch in size of property names and values") |
There was a problem hiding this comment.
This should prefer NAPI_THROW to throw as a c++ exception when it is enabled:
| Napi::Error::New(env, "Mismatch in size of property names and values") | |
| NAPI_THROW( | |
| Napi::Error::New(env, "Mismatch in size of property names and values"), | |
| Object()); |
There was a problem hiding this comment.
Addressed in push from 23b62a9 to be1b480 (diff)
| std::vector<napi_value>& propertyNames, ///< Property names | ||
| std::vector<napi_value>& propertyValues ///< Property values |
There was a problem hiding this comment.
This could take const vectors:
| std::vector<napi_value>& propertyNames, ///< Property names | |
| std::vector<napi_value>& propertyValues ///< Property values | |
| const std::vector<napi_value>& propertyNames, ///< Property names | |
| const std::vector<napi_value>& propertyValues ///< Property values |
There was a problem hiding this comment.
Hi @legendecas ,
Thanks for your patience :)
This cannot be const because using the .data() would then return const napi_value*, but node_api_create_object_with_properties does not accept const-qualified pointers:
../../../napi-inl.h:1651:7: error: no matching function for call to 'node_api_create_object_with_properties'
node_api_create_object_with_properties(env,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/kevineady/Library/Caches/node-gyp/24.14.0/include/node/js_native_api.h:56:1: note: candidate function not viable: 3rd argument ('const value_type *' (aka 'napi_value__ *const *')) would lose const qualifier
node_api_create_object_with_properties(napi_env env,
node_api_create_object_with_properties(napi_env env,
napi_value prototype_or_null,
napi_value* property_names,
napi_value* property_values,
size_t property_count,
napi_value* result);Might have been an oversight of the Node-API for node_api_create_object_with_properties...?
Eg. Function::New can accept an const std::vector<napi_value>& args, but that is because the underlying Node-API napi_new_instance accepts a const napi_value* argv.
There was a problem hiding this comment.
You are right, I think this can be a follow-up. node_api_create_object_with_properties is actually read-only on property_names and property_values, so it should be safe to add the const qualifiers.
In the interim, I think we could also use const_cast because this does not write into the pointee so this does not incur any runtime behavior change. But we could do it after updating node_api_create_object_with_properties.
23b62a9 to
be1b480
Compare
Add
Object::Newoverload utilizingnode_api_create_object_with_properties