search

mapbox / node-cpp-skel

Skeleton for bindings to C++ libraries for Node.js using node-addon-api
Creative Commons Zero v1.0 Universal
72 stars 10 forks source link

N-api (extra) #163

Closed artemp closed 2 years ago

artemp commented 4 years ago

Use std::string() operator for Napi::String to std::string conversion where appropriate to reduce verbosity but add comment about explicit Utf8Value() method

auto str = val.As<Napi::String>().Utf8Value(); //explicit convertion
std::string = val.As<Napi::String>();// operator std::string()
springmeyer commented 2 years ago

👋🏼 @artemp just noticed this PR. It looks to me like this pre-dates the n-api support landing in master. And so despite it looking like a very big set of changes the only meaningful thing is the last commit (https://github.com/mapbox/node-cpp-skel/pull/163/commits/096ef38aedd4f7026e43bc116cfa5110600e11ab). Is that right? If so, does it still make sense to land the string change improvement? If so, perhaps you could re-create a new PR against latest main branch?

artemp commented 2 years ago

👋🏼 @artemp just noticed this PR. It looks to me like this pre-dates the n-api support landing in master. And so despite it looking like a very big set of changes the only meaningful thing is the last commit (096ef38). Is that right? If so, does it still make sense to land the string change improvement? If so, perhaps you could re-create a new PR against latest main branch?

@springmeyer - Looks like https://github.com/mapbox/node-cpp-skel/commit/096ef38aedd4f7026e43bc116cfa5110600e11ab is already in main e.g

https://github.com/mapbox/node-cpp-skel/blob/main/src/object_async/hello_async.cpp#L155-L159 https://github.com/mapbox/node-cpp-skel/blob/main/src/object_sync/hello.cpp#L24-L28 unless I'm missing something ?

springmeyer commented 2 years ago

Ah cool, thanks @artemp - so then this can be closed without any action. Thanks!