update for latest n-api changes #140

mhdawson · 2017-09-19T00:23:09Z

There is a better way to handle the changes for async hooks, this gets it compiling/working but I know its not right @jasongin can you suggest what we should be doing in terms of properly supporting the new async hook parameters.

mhdawson · 2017-09-19T00:28:37Z

@gabrielschulhof the other thing of note in this is that I had to add uv.h and node.h to node_internals.h because of refactoring that took place in the node files.

I'm wondering who we could have built the wrapper without the vm as node_api.cc would have needed uv.h and node.h ?

Maybe its just late and I'm tired though ?

jasongin · 2017-09-19T21:57:01Z

Following is an outline of the updates required for the async hooks changes:

Define a C++ type for the new napi_async_context type; its initialization calls napi_async_init() and destruction calls napi_async_delete(). (Think carefully about how the async context lifetime is managed using C++ constructor/destructor semantics.)
Add an async context parameter to all MakeCallback() function overloads.
Add async resource and resource-name parameters to the AsyncWorker constructor and pass those to napi_create_async_work(). (Optionally the async resource object can be the same as the receiver parameter.)
Change AsyncWorker::OnOK() and AsyncWorker::OnError() callbacks to not use MakeCallback(). An ordinary function call (_callback::Call()) is now correct.

Since it's not trivial, it's probably better to do as a separate PR.

mhdawson · 2017-09-20T18:20:15Z

@jasongin so are you suggesting that we land this PR as is and then do a follow on PR to cover the steps you have outline ? That would make sense to me. It would let us address the current breakage so that we minimize the time people can't use the wrapper with master (and 8.6 when goes out soon)

jasongin · 2017-09-20T18:24:39Z

Yes.

mhdawson · 2017-09-21T14:01:10Z

Ok then @jasongin need a review on this PR so I can land.

jasongin · 2017-09-21T15:57:26Z

napi-inl.h

+                                 ModuleRegisterCallback registerCallback) {
+  return details::WrapCallback([&] {
+    return napi_value(registerCallback(Napi::Env(env),
+                      Napi::Object(env, exports)));


nit: The last line here should be indented further to align with the previous argument.

Or, the explicit napi_value() constructor can be omitted because that conversion can be implicit.

jasongin · 2017-09-21T17:06:24Z

napi-inl.h

+
+  napi_value resource_id;
+  napi_status status = napi_create_string_latin1(
+    _env, "generic", -1, &resource_id);


I don't like this much, but it's fine for now. The follow-up changes will move this resource name string to a parameter of the AsyncWorker constructor.

mhdawson · 2017-09-21T20:34:12Z

Will land and address @jasongin comments.

PR-URL: #140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

mhdawson · 2017-09-21T20:39:31Z

Landed as 8b66730

This change is initiated from nodejs#140 (comment).

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>

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

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>

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

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>

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

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>

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

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>

mhdawson added 2 commits September 20, 2017 14:38

src: update to latest n-api

b2bb320

src: update for latest n-api changes

0dc4a21

mhdawson force-pushed the breakfix1 branch from 99fac41 to 0dc4a21 Compare September 20, 2017 18:47

jasongin reviewed Sep 21, 2017

View reviewed changes

jasongin approved these changes Sep 21, 2017

View reviewed changes

mhdawson added a commit that referenced this pull request Sep 21, 2017

src: update to latest n-api

8b66730

PR-URL: #140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

mhdawson closed this Sep 21, 2017

romandev mentioned this pull request Feb 19, 2018

Add support for callback scopes #223

Closed

romandev added a commit to romandev/node-addon-api that referenced this pull request Apr 28, 2018

Add resource_name and resource parameters to AsyncWorker constructor

ee20c3e

This change is initiated from nodejs#140 (comment).

This was referenced Apr 30, 2018

Implement AsyncContext class #252

Closed

Add resource_name and resource parameters to AsyncWorker constructor #253

Closed

romandev added a commit to romandev/node-addon-api that referenced this pull request May 20, 2018

Add resource_name and resource parameters to AsyncWorker constructor

6df78ce

This change is initiated from nodejs#140 (comment).

romandev added a commit to romandev/node-addon-api that referenced this pull request May 20, 2018

Add resource_name and resource parameters to AsyncWorker constructor

d9dbe05

This change is initiated from nodejs#140 (comment).

kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022

src: update to latest n-api

583d147

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022

src: update to latest n-api

f839e2f

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022

src: update to latest n-api

42e8373

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023

src: update to latest n-api

c79587c

PR-URL: nodejs/node-addon-api#140 Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update for latest n-api changes #140

update for latest n-api changes #140

mhdawson commented Sep 19, 2017 •

edited

Loading

mhdawson commented Sep 19, 2017

jasongin commented Sep 19, 2017

mhdawson commented Sep 20, 2017 •

edited

Loading

jasongin commented Sep 20, 2017

mhdawson commented Sep 21, 2017

jasongin Sep 21, 2017

jasongin Sep 21, 2017

mhdawson commented Sep 21, 2017

mhdawson commented Sep 21, 2017

update for latest n-api changes #140

update for latest n-api changes #140

Conversation

mhdawson commented Sep 19, 2017 • edited Loading

mhdawson commented Sep 19, 2017

jasongin commented Sep 19, 2017

mhdawson commented Sep 20, 2017 • edited Loading

jasongin commented Sep 20, 2017

mhdawson commented Sep 21, 2017

jasongin Sep 21, 2017

Choose a reason for hiding this comment

jasongin Sep 21, 2017

Choose a reason for hiding this comment

mhdawson commented Sep 21, 2017

mhdawson commented Sep 21, 2017

mhdawson commented Sep 19, 2017 •

edited

Loading

mhdawson commented Sep 20, 2017 •

edited

Loading