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.

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

Suggestion: Promote use of N-API for native addons #52

Open
gabrielschulhof opened this issue Dec 6, 2018 · 14 comments
Open

Suggestion: Promote use of N-API for native addons #52

gabrielschulhof opened this issue Dec 6, 2018 · 14 comments
Labels
stale? This issue is dusty, please take a look and consider closing

Comments

@gabrielschulhof
Copy link

In light of nodejs/node#22754 (comment) we may want to reach out to native addon maintainers to remind them of the benefits of porting their addon to N-API. We at @nodejs/n-api can support their efforts.

@mcollina
Copy link
Member

mcollina commented Dec 7, 2018

Absolutely, big big +1. This could be a good item in the todo list.

Have you got a list of popular modules that are going to break because of that, and how their maintainers would feel about a rewrite-to-napi PR?

@gabrielschulhof
Copy link
Author

@gabrielschulhof
Copy link
Author

@mcollina we need to follow through on some of the things we've already ported. leveldown, for example, was ported early on in the N-API design process, so it basically needs to be ported again.

@mcollina
Copy link
Member

mcollina commented Dec 7, 2018

My main concern is not about doing the work, but rather in asking the maintainer if they would accept such PR. We should not overstep them and help them only if they are ok with it.

@ralphtheninja
Copy link

ralphtheninja commented Dec 7, 2018

@gabrielschulhof @mcollina I'm currently working on a n-api implementation of leveldown. Gonna need a few days more before review. I'm guessing accepting such a PR would be quite overwhelming as a maintainer. I prefer to do it myself and maybe instead ask for help/assistance if I run into dragons. As a maintainer it gives me good incentive to learn more about the code base and the c++ side has always been sort of off-limits for me and other maintainers.

@mhdawson
Copy link
Member

+1 to this suggestion as well (full disclosure I have been a supporter/participant of N-API from the start :)).

@mogill
Copy link
Contributor

mogill commented Dec 23, 2018

Porting from NAN to N-API is probably not an awareness issue but more of a cost-benefit problem. NAN and N-API are functionally equivalent: unless the module succumbs to NAN code-rot there is no value to investing time porting from NAN to N-API, especially if the port is non-trivial.

Of course NAN modules are already breaking, which leads back to the core issue of this package-maintenance issue: if a module's maintainer already isn't keeping up with functionality issues, the NAN to N-API conversion is, at best, a treadmill. It makes little sense for an unpaid, uninterested package maintainer to spend hours or days learning N-API when N-API experts are already available to help. For those modules awareness of where to get help doing the work is far more valuable than awareness of the tools and details of the work itself.

The fact Node.js' maintainers already support NAN/N-API conversion for some native modules is news to me. What makes a module eligible for support from @nodejs/n-api? Can package maintainers request help? If so, what is that process?

@ghinks
Copy link
Contributor

ghinks commented Dec 24, 2018

I think I'm echoing @mcollina and @mhdawson in that we need to identify the modules first. But this is great, I may at last be able to use some of my lost decades of C++ skills again.
Maybe we should just identify the top compiled modules and offer assistance to convert them as previously suggested. It always has to start with diplomacy.

@gabrielschulhof
Copy link
Author

@ghinks we have a list of the biggest hitters in nodejs/abi-stable-node#346 (comment).

@ghinks
Copy link
Contributor

ghinks commented Jan 1, 2019

excellent, I took a look at npm time (as it looked easier, and its been a while since I wrote C++, so I wanted easier). I forked and created a branch and migrated it to napi here. It seems to be all working on napi but as we form teams I'm sure we can get better at reviewing the api changes. I'll need help setting it up on travis for all the different OSs. But it is building and passing all the tests on linux on various node versions.
When we have our next meeting I'm sure we can get organized about how to do this efficiently going forwards, like contacting owners etc.

@zeke
Copy link
Contributor

zeke commented Jan 1, 2019

Here's a(n outdated) list of top dependents of nan by dependent count: https://github.com/nice-registry/native-modules#readme -- This could be a useful list for prioritizing which modules authors to reach out to.

There was an effort among to native module maintainers a while back to consolidate work into an org: https://github.com/prebuild -- all the discussion around it happened in this issue: prebuild/welcome#1

@ghinks
Copy link
Contributor

ghinks commented Jan 15, 2019

I have taken a suggestion from @mhdawson and contacted a friendly at the serialport package and am looking into converting that NAN interfaces to use NAPI. I have exchanged emails with the maintainer and they are a candidate customer.

@ghinks
Copy link
Contributor

ghinks commented Jan 28, 2019

I have additional findings which I am going to follow up with the n-api group and the maintainers group. There are some particular findings I have about updating via node-addon-api to replace nan and direct lib uv usage.

@jonchurch jonchurch added the stale? This issue is dusty, please take a look and consider closing label Jul 29, 2021
@jonchurch
Copy link
Contributor

There's some really good stuff in this issue, marking as stale? to hopefully get someone to come dust this off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale? This issue is dusty, please take a look and consider closing
Projects
None yet
Development

No branches or pull requests

8 participants