Add resource_name and resource parameters to AsyncWorker constructor#253
Add resource_name and resource parameters to AsyncWorker constructor#253romandev wants to merge 1 commit intonodejs:masterfrom
Conversation
| 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?
| (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)
| 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).
romandev
left a comment
There was a problem hiding this comment.
I addressed your comments.
Could you please review this patch?
| const events = []; | ||
| const hook = async_hooks.createHook({ | ||
| init(asyncId, type, triggerAsyncId, resource) { | ||
| if (type === 'TestResource'){ |
| (async() => { | ||
| await test(require(`./build/${buildType}/binding.node`)); | ||
| await test(require(`./build/${buildType}/binding_noexcept.node`)); | ||
| })(); |
| 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 <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
|
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 <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
This change is initiated from nodejs/node-addon-api#140 (comment). PR-URL: nodejs/node-addon-api#253 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@NickNaso.local>
This PR is initiated from #140 (comment).