search

nodejs / abi-stable-node

Repository used by the Node-API team to manage work related to Node-API and node-addon-api
239 stars 47 forks source link

Do we need some additional AsyncWorker variants #373

Closed mhdawson closed 4 years ago

mhdawson commented 5 years ago

See https://github.com/nodejs/node-addon-api/issues/473

gabrielschulhof commented 5 years ago

During today's meeting @NickNaso and @gabrielschulhof agreed that it is reasonable to add node-addon-api equivalents to the Nan::AsyncWorker variants. We could implement them using napi_threadsafe_function.

gabrielschulhof commented 5 years ago

A proposal for an AsyncWorker that uses the Promise interface is made here: https://github.com/nodejs/node-addon-api/issues/231#issuecomment-496529366

mhdawson commented 5 years ago

So we have an issue for ProgressWorker but not PromiseWorker. We need to open an issue in node-addon-api for creating a PromiseWorker equivalent.

mhdawson commented 5 years ago

Seems like the only variant we need to prioritize ProgressWorker as Nan does not have PromiseWorker. Since we have nodejs/node-addon-api#473 I think we are good in terms of issues in the node-addon-api. We have tagged it as help needed for now.

legendecas commented 5 years ago

Just found https://github.com/nodejs/node-addon-api/pull/477 noted that node-addon-api would prefer additional packages on topics not just a wrapper of n-api. Would AsyncWorker variants be considered merely additions to node-addon-api usage patterns but not wrappers of n-api?

mhdawson commented 5 years ago

@legendecas the exception for "additions" that we've made while developing node-addon-api is that in order to facilitate transition from nan to node-addon-api, if there is functionality in nan then we've tried to have corresponding functionality in node-addon-api where it makes sense.

mhdawson commented 4 years ago

PR to add AsyncProgressWorker: https://github.com/nodejs/node-addon-api/pull/529

mhdawson commented 4 years ago

There is also AsyncProgressQueueWorker which should be addressed before we close this issue. It is part of nan so we should consider adding it to N-API as well.

This is a PR doing that: https://github.com/nodejs/node-addon-api/pull/585

mhdawson commented 4 years ago

PR landed, closing