Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AsyncWorker: add GetResult() method #512

Closed
wants to merge 2 commits into from
Closed

AsyncWorker: add GetResult() method #512

wants to merge 2 commits into from

Conversation

KevinEady
Copy link
Contributor

Adds an overridable GetResult() method, providing arguments to the callback
invoked in OnOK().

Re: #231 (comment)

@KevinEady
AsyncWorker: add GetResult() method
ab421b9 
Adds an overridable `GetResult()` method, providing arguments to the callback
invoked in `OnOK()`.

Re: nodejs#231 (comment)
@KevinEady
Copy link
Contributor Author

Hey @mhdawson / @gabrielschulhof ,

This is the implementation of the GetResult() method we discussed in yesterday's meeting. There are two things that I did change

  1. The method takes an Napi::Env. I thought it would be beneficial to provide this, so the implementer can create the arguments (since the callback and receiver are private, cannot get env from those)
  2. Instead of an std::initializer_list, I used an std::vector. When using the initializer list, there were issues with scoping / binding the initializer list to a temporary array (if doing something eg return { Boolean::New(env, true) }). I think the vector implementation should be fine, since Function::Call already had a matching prototype.

Let me know what you guys think 👍

mhdawson
mhdawson reviewed Jul 16, 2019
View reviewed changes
napi.h Show resolved Hide resolved
mhdawson
mhdawson reviewed Jul 16, 2019
View reviewed changes
napi-inl.h Outdated
}
// The OnExecute method receives an napi_env argument. However, do NOT
// use it within this method, as it does not run on the main thread and cannot
// access the napi_env.
Copy link
Member

@mhdawson mhdawson Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use the following instead:

as it does not run on the main thread and must not run any method that would cause
JavaScript to run. In practice, this means that almost any use of napi_env will be incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

mhdawson
mhdawson reviewed Jul 16, 2019
View reviewed changes
test/asyncworker.cc Outdated Show resolved Hide resolved
mhdawson
mhdawson approved these changes Jul 18, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

@gabrielschulhof just want your input on the change to an array as well.

@mhdawson mhdawson mentioned this pull request Jul 18, 2019
Closed
2 tasks
gabrielschulhof
gabrielschulhof approved these changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

NickNaso
NickNaso approved these changes Jul 19, 2019
View reviewed changes
Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

mhdawson pushed a commit that referenced this pull request Jul 22, 2019
@KevinEady @mhdawson
AsyncWorker: add GetResult() method
717c9ab 
Adds an overridable `GetResult()` method, providing arguments
to the callback invoked in `OnOK()`.

Refs: #231 (comment)
PR-URL: #512
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
@mhdawson
Copy link
Member

Landed as 717c9ab

@mhdawson mhdawson closed this Jul 22, 2019
@JckXia JckXia mentioned this pull request Mar 23, 2021
Closed
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
AsyncWorker: add GetResult() method
43a7d5d 
Adds an overridable `GetResult()` method, providing arguments
to the callback invoked in `OnOK()`.

Refs: nodejs/node-addon-api#231 (comment)
PR-URL: nodejs/node-addon-api#512
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
AsyncWorker: add GetResult() method
b90dc60 
Adds an overridable `GetResult()` method, providing arguments
to the callback invoked in `OnOK()`.

Refs: nodejs/node-addon-api#231 (comment)
PR-URL: nodejs/node-addon-api#512
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
AsyncWorker: add GetResult() method
8c768f5 
Adds an overridable `GetResult()` method, providing arguments
to the callback invoked in `OnOK()`.

Refs: nodejs/node-addon-api#231 (comment)
PR-URL: nodejs/node-addon-api#512
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
AsyncWorker: add GetResult() method
667b0ad 
Adds an overridable `GetResult()` method, providing arguments
to the callback invoked in `OnOK()`.

Refs: nodejs/node-addon-api#231 (comment)
PR-URL: nodejs/node-addon-api#512
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: NickNaso <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@NickNaso NickNaso NickNaso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KevinEady @mhdawson @NickNaso @gabrielschulhof