GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||
GeezerVison is showing github.com — n-api: implement async helper methods by boingoing · Pull Request #12250 · nodejs/node Conversation
|
|
@jasongin @digitalinfinity @mhdawson @sampsongao |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Would it be reasonable to use |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Why would you do this in napi instead of nan? Seems like feature creep. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@bnoordhuis See also #11975 (comment). I am still not 100 % sure about this either… |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@bnoordhuis there was also a comment from @kkoopa on this issue: nodejs/abi-stable-node#204 In response to discussion about removing from the original PR (which we did temporarily, and are now adding back in): |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@kkoopa There isn't an ABI issue blocking us from using |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The issue and comments @mhdawson is referencing above are at nodejs/abi-stable-node#204 (comment) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yes that is what I was wondering about. If the class is not exposed, it is fine as is.
…On April 6, 2017 11:09:32 PM GMT+03:00, Taylor Woll ***@***.***> wrote:
>Would it be reasonable to use unique_ptr instead of handing out raw
pointers in the Work class? Is there an ABI issue?
@kkoopa There isn't an ABI issue blocking us from using `unique_ptr`
wrapping `Work` instances and handing those out as `napi_async_work`
pointers, if that's what you meant. Since the `Work` class is not
exposed outside of napi internals, we aren't worried about users
messing-up the state via these opaque `napi_async_work` pointers. We
use the same pattern for returning `Reference` instances via `napi_ref`
opaque pointers via `napi_create_reference`. That's the main reason I
have used the same pattern here.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#12250 (comment)
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nit: redundant space before function
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It is not redundant, it is part of the string on the line above.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can you have this test case queue the work multiple times and verify that that works? You'll probably have to change the complete handler to not delete the work after every item but only when all work has been completed
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why was this moved?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I put the napi_callback typedef near napi_async_execute_callback and other callback typedefs. But napi_async_complete_callback takes a napi_status so all the callbacks should be located down below the enum (at least). However, napi_property_descriptor has napi_callback members so it also had to move down in the file.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Either
errorMessage = errorMessage == nullptr ? "empty error message" : errorMessage;
or
errorMessage = errorMessage || "empty error message";
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Good point. This will be refactored via #12248 so I didn't pull all of that in for this change. Will make this change, though.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We use snake_case for C++ code.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing. I fixed this via 1b210fe
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
CI run: https://ci.nodejs.org/job/node-test-pull-request/7300/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Link failures: ln -s /usr/local/bin/node Please fix up and make sure to run make lint |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@mhdawson Pushed 85ca55a to fix these lint errors. Are the same rules used when we run Anyway, could you kick off a new CI, please? :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I assume it should be the same across platforms so not sure what might have happend: New CI: https://ci.nodejs.org/job/node-test-pull-request/7309/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry your other PR made it in first can you do a rebase and the I'll give it one more shot. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test.
|
@mhdawson No worry, I knew one of them would get in first which would conflict with the other. I did the rebase and running test locally to see if I can catch any kind of lint errors on my end before updating the PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ok, If you can do it today I'll check in later tonight to see if we can get it landed as I'm pretty booked tomorrow. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@mhdawson Thanks for baby-sitting this PR. I pushed the rebase via 35a0287. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 9decfb1 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test. PR-URL: #12250 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
|
:) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks all! 😃 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
do we need to tag this with the right labels to be picked up for next RC ? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test. PR-URL: nodejs#12250 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Based on the async methods we had in abi-stable-node before the napi feature landed in node/master. Changed this set of APIs to handle error cases and removed a lot of the extra methods we had for setting all the pieces of napi_work opting instead to pass all of those as arguments to napi_create_async_work as none of those parameters are optional except for the complete callback, anyway. Renamed the napi_work struct to napi_async_work and replace the struct itself with a class which can better encapsulate the object lifetime and uv_work_t that we're trying to wrap anyway. Added a napi_async_callback type for the async helper callbacks instead of taking raw function pointers and make this callback take a napi_env parameter as well as the void* data it was already taking. Call the complete handler for the async work item with a napi_status code translated from the uvlib error code. The execute callback is required for napi_create_async_work, though complete callback is still optional. Also added some async unit tests for addons-napi based on the addons/async_hello_world test. Backport-PR-URL: #19447 PR-URL: #12250 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.