N-API Support for node.bcrypt.js

[aruneshchandra]

The recently announced Node 8 has a new experimental feature called N-API which is aimed at reducing maintenance cost for node native addons. Checkout this blog for more details on its benefits.

node.bcrypt.js is one of the top 30 native modules by dependency count, and in order to help the Node.js community make this important transition to N-API, we are hoping you will be able to work with us in order to port node.bcrypt.js to support N-API. Your support and feedback is important in making this effort a success.

I am part of the N-API working group and and would like to talk to you about how you can can get started with N-API and what help we might be able to provide. I'm available to talk on the phone individually if you'd like. Alternatively, if you prefer, feel free to jump in our WG meeting which happens every Thursday at 1.30 Eastern / 10.30 Pacific US time, to discuss any issues.

Here’s a video of N-API in action
https://www.youtube.com/watch?v=nmXhJ88nZsk

[recrsn]

I tried to get it work. We heavily depend on AsyncWorker but I couldn't find much docs about it.
And due to other commitments, I could not spend much time on it.

[aruneshchandra]

You are right there is not a very detailed documentation on AsyncWorker.. however the following header file can help you understand the usage
https://github.com/nodejs/node-addon-api/blob/master/napi.h#L1362

Alternatively, do you mind talking to us during our Weekly WG meeting which happens every Thursday at 1.30 Eastern / 10.30 Pacific US time, to discuss some ways we can help you to get started on this port.

[aruneshchandra]

//cc @kfarnung - please update the documentation for AsyncWorker

[NickNaso]

Hi,
today I completed the porting of bcrypt from NAN to N-API using node-addon-api the code is here https://github.com/NickNaso/node.bcrypt.js/tree/node-addon-api
All tests successfully pass. Maybe you can consider to integrate the code and with some additional work on documentation and package.json we can publish the N-API version of bcrypt I will be happy to collaborate.

[aruneshchandra]

//cc: @agathver, @mhdawson, @digitalinfinity

[recrsn]

Great work @NickNaso

Please open a PR so that we can continue to iterate there

[NickNaso]

Hi @agathver
I think that we need to create a branch so I can execute the PR on it. What do you think?

[recrsn]

@NickNaso Yes, created a branch called "napi".

[mhdawson]

@NickNaso thanks for doing this.

If you don't mind some info on how long it took you (The commits are over 4-5 days) to do the port along with highlighting any things you found more tricky would be good as data for people looking at doing other ports.

[NickNaso]

Hi everyone,
I just returned at home I really happy to share my experience on porting bcrypt to N-API
All the translation process has been taken 5 days of my spare time. In total 10 / 15 hours of work.
The most difficult task was to create and use AsyncWorker in particular I don't know how to call the callback from the OnOk method but I saw at code here https://github.com/nodejs/node-addon-api/blob/master/napi.h#L1433 and to understand how to better use it I looked at this porting https://github.com/sampsongao/leveldown/tree/napi
There aren't examples of usage AsyncWorker here https://github.com/nodejs/abi-stable-node-addon-examples
To understand how to handle the errors I used the examples from the test https://github.com/nodejs/node-addon-api/blob/master/test/error.cc
As reference I followed the N-API documentation here https://nodejs.org/dist/latest-v8.x/docs/api/n-api.html
It was not very difficult but with more useful documentation could be more simple and fast.

[NickNaso]

Hi,
today I updated the code and just fixed some compilation error that I received on Windows platform and now all work fine on macOS - linux - Windows with Node.js version 8, but in Node.js version previous the 8 the module is compiled well but I receive an error when I require it.
Just post an example

[digitalinfinity]

Hi @NickNaso to clarify- you're using the binary that was compiled against NAPI as implemented in Node 8.6+? On downlevel, I believe the story is you need to use the node-addon-api package for compat with old node versions. Which version of Node do you get the above error in?

[NickNaso]

Hi @digitalinfinity
I'm using node-addon-api and when I switch to node 7 - 6 - 5 - 4 I get the reported error. I'm able compile the addon without the error for different Node.js version, but when I import the addon I get the reported error. If you want more info you can see what happen from the CI:
Windows: https://ci.appveyor.com/project/defunctzombie/node-bcrypt-js-pgo26/build/1.0.50
Linux & macOS: https://travis-ci.org/kelektiv/node.bcrypt.js/builds/288858606?utm_source=github_status&utm_medium=notification

[NickNaso]

Hi @agathver, @mhdawson, @digitalinfinity @aruneshchandra
Now all tests successfully passed on Node.js version 4 - 5 - 6 - 7 - 8, but in package.json file I insert node-addon-api as reported below:

"dependencies": {
    "node-addon-api": "git+https://github.com/nodejs/node-addon-api.git"
}

If I use the node-addon-api from npm I continue to get error importing the addon.

[recrsn]

@NickNaso Lets continue the discussion in #551

[NickNaso]

Hi @aruneshchandra @mhdawson @agathver @digitalinfinity,
with new release of node-addon-api all work fine. Take a look at #551 where I proposed a way to go forward and ship the N-API version of brcypt.

[mhdawson]

@NickNaso good to hear :)

[NickNaso]

Hi everyone,
I'm writing to know if you want publish the N-API version of bcrypt and if you want some help I'm available. In the mean time I conducted some little experiments about performance considering the function's execution time and on my machine the N-API version is average is 2% faster. After that I tested it on a web application and all worked fine and the performance continue to be at the same level. I recently deployed a web application in production where I used my work (I'm installing it from a git repo) and all went good.
In the next meeting we will talk about the node-pre-gyp integration.
Good work to all of you and keep me updated about your intention on publish the module.

[mhdawson]

@agathver just wondering if we can get a tagged version so that Nick can use the module in production ?

[recrsn]

@mhdawson
[email protected] is now on npm as bcrypt@n-api

[NickNaso]

Thank you @agathver I just started using it on https://www.lexgenda.com/ in production and until now it worked like previous version. I will keep you updated.

[mhdawson]

@agathver thanks!

[recrsn]

@NickNaso node-pre-gyp has got napi support. Will you be willing to contribute a patch adding it to the napi branch?

[NickNaso]

@agathver I will do it in this weekend and I will open a PR on napi branch.

[NickNaso]

Hi @agathver,
I worked on porting the updates on the NAN version of the add-on to the N-API release. I added the support for node-pre-gyp.
Please when you have time see the PR: #698

[NickNaso]

Hi everyone,
Is there a plan to switch to N-API for this module? In case how can I help?

[recrsn]

The plan is to maintain both versions of the module for now, till Node 8 remains supported.

[NickNaso]

Ok I will try to maintain the N-API version in sync with the classical version.

[DRoet]

Any plans for a 4.x.x release that uses N-API by default? now that node 8 is almost EOL.

[recrsn]

That's on the roadmap for this November.

[recrsn]

bcrypt is now using N-API by default

[mhdawson]

@agathver thanks for the update !