nodejs / abi-stable-node

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

Investigate N-API helping to avoid future breakage like in https://github.com/nodejs/node/pull/22754#issuecomment-425072975 #346

Closed mhdawson closed 5 years ago

mhdawson commented 6 years ago

https://github.com/nodejs/node/pull/22754#issuecomment-425072975

Hitesh suggested we should look at the modules that were broken and see if they are good candidates for N-API as its could have prevented this kind of breakage.

Next step is for Hitesh to look at the modules to identify if there are any good candidates After that we may reach out to maintainers and offer mentoring.

digitalinfinity commented 6 years ago

@joaocgreis helpful compiled a list of the modules that were impacted by the API change. Here are my notes:

Module Notes
level doesn't appear to be a native module, wrapper for leveldown?
serialport we've started a port but it probably needs to be revived - Glenn is engaging here.
ref Already ported to n-api by @addaleax
iconv Easy port to N-API. We should engage here. (https://github.com/bnoordhuis/node-iconv/pull/189 needs follow-up work or adoption) - now we have this new PR https://github.com/bnoordhuis/node-iconv/pull/196
bcrypt Already ported to n-api by @NickNaso -- Mantainers will switch to N-API when Node.js v8 will go aut of maintenance
leveldown We have an old N-API port, but we should re-engage here (https://github.com/Level/leveldown/pull/540) N-API port available in v5.0 which is the next version of leveldown
microtime Easy port to N-API. We should engage here @NickNaso has a port at https://github.com/wadey/node-microtime/issues/56 Porting merged to master and the version 3.0.0 of the add-on has been published and now use N-API
time Easy port to N-API. We should engage here (Glenn has a PR in)
sqlite3 PR in progress here, it looks like we need to get back to @springmeyer
node-report Not a good candidate for N-API - Being merged into core
zeromq @rolftimmermans has a next generation version here that is on the roadmap to be merged back to master!
NickNaso commented 6 years ago

Hi everyone, I already worked on microtime some months ago. See https://github.com/wadey/node-microtime/issues/56

mhdawson commented 6 years ago

@joaocgreis Thanks for the list.

In terms of sqlite3 @anisha-rohra has been working on that and we are down to one problem. Slowing us down is that Anisha is back at school and so only has a smaller amount of available time as a student on call.

In terms of node-report we have a PR open to meld it into core which would remove it from this kind of problem as the testing would be part of node core itself.

mhdawson commented 6 years ago

In terms of serialport, it was just the very start of the port and I believe the code base has probably changed a lot since then as well. All to say its probably a restart from scratch in that case.

vweevers commented 5 years ago

Regarding leveldown, see https://github.com/nodejs/package-maintenance/issues/52#issuecomment-445398261

ralphtheninja commented 5 years ago

doesn't appear to be a native module, wrapper for leveldown?

@digitalinfinity Yes!

ghinks commented 5 years ago

I had a stab at npm time and forked it with a conversion from nan to napi here

ghinks commented 5 years ago

I raised a PR for the npm time module. A couple of things I have learned from this first PR.

mhdawson commented 5 years ago

@ghinks it should for 6.X as well, although in the case of both 6.x and 8.x its only after a particular semver minor version.

gabrielschulhof commented 5 years ago

There's another module we should consider helping to get ported:

https://github.com/nospaceships/node-raw-socket/

https://www.npmjs.com/package/raw-socket (445145 weekly downloads)

The module net-ping depends on it, which itself has just as many downloads.

Filed an issue: https://github.com/nospaceships/node-raw-socket/issues/54

ghinks commented 5 years ago

yes, let me take a look. I am hopefully going to meet the other maintainers for the serialport module. That module uses lots of libuv and is looking more like a re-write.

ghinks commented 5 years ago

@mhdawson @gabrielschulhof I would like some advice although not sure if this is the correct forum. The node-serial-port module that I am looking at converting from nan makes a lot of use libuv for polling file descriptors.

node serial port libuv example

Keeping this breaks the ideals of N-API use. But re-implementing the event loop just for this purpose is also not a good idea. I'm just wondering what has been done in the past.

mhdawson commented 5 years ago

@ghinks I think opening a new issue in abi-stable-node would be a better place to have the discussion. If it has a heavy dependency on libuv it may not be a good N-API candidate (my impression from the maintainers was that it was not the case, h/w interaction was done at the OS specific level. I might not have understood or maybe that changed). The goal was to hit the 80 of the 80/20 rule. Having said that we have had some discussion about how we take advantage of the fact that libuv does not change that often so understanding what is used and why is good. We might also test out the ideas we've had around libuv/abi numbers on the node serial port maintainers. So if you can open a new issue to discuss I think that would be best.

NickNaso commented 5 years ago

Hi everyone I want report a tweet that I found today Screenshot 2019-04-29 at 20 00 59

https://twitter.com/emilbayes/status/1122787284372340737

The maintainer of sodium-native announce that the next version will be implemented by N-API.

NickNaso commented 5 years ago

Hi everyone I want report that the next version of rocksdb will be based on N-API see:

Issue that track the N-API refactor: https://github.com/Level/rocksdb/issues/100

PR of the rewrite using N-API: https://github.com/Level/rocksdb/pull/111

In the same PR you can see the first performance result and it seems that N-API version is faster than other on writing operation. https://github.com/Level/rocksdb/pull/111#issuecomment-495607161

NickNaso commented 5 years ago

Hi everyone, I want report that farmhash v3.0.0 is implemented by N-API.

gabrielschulhof commented 5 years ago

https://github.com/LinusU/fs-xattr/ is N-API as of version 0.3.0. (~8000 dl/week on npm).

gabrielschulhof commented 5 years ago

https://github.com/websockets/bufferutil/ is N-API at least as of version 4.0.1 (~76900 dl/week on npm).

gabrielschulhof commented 5 years ago

https://github.com/abbr/deasync/ is N-API at least as of version 0.1.25 (~206822 dl/week on npm).

gabrielschulhof commented 5 years ago

Wading my way through this query:

https://github.com/search?p=3&q=napi_create_function+language%3AC+-filename%3Atest_function.c+-filename%3Abinding.c+-path%3Atest%2Faddons-napi+-path%3Atest%2Fjs-native-api+-path%3Atest%2Fnode-api&type=Code

https://github.com/search?p=11&q=napi_create_function+language%3AC+-filename%3Atest_function.c+-filename%3Abinding.c+-path%3Atest%2Faddons-napi+-path%3Atest%2Fjs-native-api+-path%3Atest%2Fnode-api+-filename%3Acompare_env.c+-user%3Aelominp&type=Code

gabrielschulhof commented 5 years ago

https://github.com/websockets/utf-8-validate is N-API at least as of version 5.0.2 (~72420 dl/week on npm).

NickNaso commented 5 years ago

I want report that the team of neon started working on N-API support. This is the PR to follow: https://github.com/neon-bindings/neon/pull/440 This will offer the possibility to implement native addon with Rust through N-API.

In the PR I found a discussion about module registration:

The reason I'm not sure we can eliminate it is the way to initialize a module requires a C macro (NAPI_MODULE_INIT) that seems to expand into non-ABI-stable details. It's worth reaching out on the N-API repo to see if they might consider creating a stable way to do this from non-C languages, but so far I can't figure out a way around it otherwise.

I proposed they to join to our weekly meeting if want to know some details about N-API that could help them with their work.

Here I report what they need to know:

I may not be able to join that call this week but I'll see. If someone happen to have the opportunity to join, I'd love if we could ask two things:

Are the napi_module type and the neon_register_module function ABI stable? They aren't documented (the only documented way to register a module is with C macros) but if they're stable, @kjvalencik has already done a proof of concept that we can use them from Rust without C glue. If they are guaranteed to be stable, could they be added to the docs?

mhdawson commented 5 years ago

We discussed this in the N-API team meeting today. We agreed that the structure/napi_module_register is ABI stable. Michael will take action to create PR to document napi_module_register better.

However, @gabrielschulhof suggest that if they are going to use 10.x and later only, its better to use the newer option which uses an exported symbol as it does not need features like library constructors so it's more general. He'll take an action to PR in more documentation for that.

gabrielschulhof commented 5 years ago

We oughta maybe look into https://www.npmjs.com/package/zeromq

gabrielschulhof commented 5 years ago

Another addon that could use a N-API port: https://github.com/libxmljs/libxmljs/

dherman commented 5 years ago

I just want to thank you all for the generous help! The well-defined symbol approach is indeed much cleaner, simpler, and more future-proof for supporting multiple workers. Thank you all!

gabrielschulhof commented 5 years ago

@dherman happy to hear it 👍

gabrielschulhof commented 5 years ago

I compiled a list of N-API modules and their npm dls/week here: https://github.com/nodejs/node/pull/28428#issuecomment-533748682 I doubt any of the modules are ones we are unaware of, but at least they're all in one place there.

NickNaso commented 5 years ago

I want report that from version 6 sse4_crc32 https://www.npmjs.com/package/sse4_crc32 use N-API through node-addon-api.

mhdawson commented 5 years ago

@NickNaso great to hear :)

mhdawson commented 5 years ago

Closing as we have new issue for this year.