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

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

Closed
mhdawson opened this issue Sep 27, 2018 · 31 comments
Assignees
Milestone

Comments

@mhdawson
Copy link
Member

nodejs/node#22754 (comment)

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
Copy link
Contributor

digitalinfinity commented Oct 2, 2018

@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. (bnoordhuis/node-iconv#189 needs follow-up work or adoption) - now we have this new PR bnoordhuis/node-iconv#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 (Level/leveldown#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 wadey/node-microtime#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
Copy link
Member

NickNaso commented Oct 2, 2018

Hi everyone,
I already worked on microtime some months ago. See wadey/node-microtime#56

@mhdawson
Copy link
Member Author

mhdawson commented Oct 2, 2018

@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
Copy link
Member Author

mhdawson commented Oct 2, 2018

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
Copy link

vweevers commented Dec 8, 2018

Regarding leveldown, see nodejs/package-maintenance#52 (comment)

@ralphtheninja
Copy link

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

@digitalinfinity Yes!

@ghinks
Copy link

ghinks commented Jan 1, 2019

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

@ghinks
Copy link

ghinks commented Jan 5, 2019

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

  • contact the package owner first and make sure they are ok with this
  • check that all the unit tests work on the master branch first
  • understand that napi works from node 8 onwards and that existing automated tests that use early versions of node may no longer work and will the owner be ok with this

@mhdawson
Copy link
Member Author

mhdawson commented Jan 7, 2019

@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
Copy link
Collaborator

gabrielschulhof commented Feb 5, 2019

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: nospaceships/node-raw-socket#54

@ghinks
Copy link

ghinks commented Feb 5, 2019

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
Copy link

ghinks commented Feb 7, 2019

@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
Copy link
Member Author

mhdawson commented Feb 7, 2019

@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
Copy link
Member

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
Copy link
Member

NickNaso commented May 20, 2019

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: Level/rocksdb#100

PR of the rewrite using N-API: Level/rocksdb#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.
Level/rocksdb#111 (comment)

@NickNaso
Copy link
Member

NickNaso commented Jul 1, 2019

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

@gabrielschulhof
Copy link
Collaborator

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

@gabrielschulhof
Copy link
Collaborator

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

@gabrielschulhof
Copy link
Collaborator

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

@gabrielschulhof
Copy link
Collaborator

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

@NickNaso
Copy link
Member

NickNaso commented Sep 13, 2019

I want report that the team of neon started working on N-API support. This is the PR to follow: neon-bindings/neon#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
Copy link
Member Author

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
Copy link
Collaborator

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

@gabrielschulhof
Copy link
Collaborator

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

@dherman
Copy link

dherman commented Sep 21, 2019

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
Copy link
Collaborator

@dherman happy to hear it 👍

@gabrielschulhof
Copy link
Collaborator

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

@NickNaso
Copy link
Member

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

@mhdawson
Copy link
Member Author

@NickNaso great to hear :)

@mhdawson
Copy link
Member Author

Closing as we have new issue for this year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants