Add resource_name and resource parameters to AsyncWorker constructor #253
Conversation
napi.h
Outdated
| AsyncWorker(const Object& receiver, | ||
| const Function& callback, | ||
| const char* resource_name, | ||
| const Object& resource); |
There was a problem hiding this comment.
Overall looks good but do we need to add explicit (like there was for the existing methods) to avoid unwanted conversions?
There was a problem hiding this comment.
I followed your comment here but I didn't find any guidance for this in node.js project. In my humble opinion, it might be better to allow the explicit keyword only for the single argument constructors. If we do that, we can have the advantage of using list initialization.
void hello(AsyncWorker worker) {
...
}
hello({ callback, "test" });FYI, many other projects(e.g. Chromium) are also following the rule.
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions
WDYT?
test/asyncworker.js
Outdated
| (async() => { | ||
| await test(require(`./build/${buildType}/binding.node`)); | ||
| await test(require(`./build/${buildType}/binding_noexcept.node`)); | ||
| })(); |
There was a problem hiding this comment.
I assume this was needed to make sure the tests ran sequentially. Could you do it without depending on await as we run the tests with older versions of Node.js that may not have support (ex 6.x I think)
test/asyncworker.js
Outdated
| const events = []; | ||
| const hook = async_hooks.createHook({ | ||
| init(asyncId, type, triggerAsyncId, resource) { | ||
| if (type === 'TestResource'){ |
|
Thank you for review. I'm on a trip until this week. So, I'll update this patch until early next week. Thank you. |
|
@romandev thanks for the update :) |
This change is initiated from nodejs#140 (comment).
test/asyncworker.js
Outdated
| const events = []; | ||
| const hook = async_hooks.createHook({ | ||
| init(asyncId, type, triggerAsyncId, resource) { | ||
| if (type === 'TestResource'){ |
test/asyncworker.js
Outdated
| (async() => { | ||
| await test(require(`./build/${buildType}/binding.node`)); | ||
| await test(require(`./build/${buildType}/binding_noexcept.node`)); | ||
| })(); |
napi.h
Outdated
| AsyncWorker(const Object& receiver, | ||
| const Function& callback, | ||
| const char* resource_name, | ||
| const Object& resource); |
There was a problem hiding this comment.
I followed your comment here but I didn't find any guidance for this in node.js project. In my humble opinion, it might be better to allow the explicit keyword only for the single argument constructors. If we do that, we can have the advantage of using list initialization.
void hello(AsyncWorker worker) {
...
}
hello({ callback, "test" });FYI, many other projects(e.g. Chromium) are also following the rule.
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions
WDYT?
This change is initiated from #140 (comment). PR-URL: #253 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>
|
Landed as 4b8918b |
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Nicola Del Gobbo <[email protected]>
This PR is initiated from #140 (comment).