bcrypt for Node.js with Node addon api #551

NickNaso · 2017-10-09T17:30:34Z

Hi,
I'm opening the PR to start iterate over the porting of bcrypt to N-API

Closes #513

recrsn

Code looks good to me. Except for a little typo in exception messages and a build issue in linux

recrsn · 2017-10-09T17:40:40Z

src/bcrypt_node.cc

    if (info.Length() < 2) {
-        Nan::ThrowTypeError("2 arguments expected");
-        return;
+        throw Napi::TypeError::New(info.Env(), "3 arguments expected");


should be 2 args expected ?

Ok I think that I’m the code there are some errors. I will change as soon as possible now I’m leaving from Vancouver to Italy and I cannot be very active

recrsn · 2017-10-09T17:47:52Z

binding.gyp

+      },
+      "xcode_settings": {
+        "CLANG_CXX_LIBRARY": "libc++",
+        'GCC_ENABLE_CPP_EXCEPTIONS': 'YES'


We need same settings in linux too.

There is additional cases for windows that needs to be handled

recrsn · 2017-10-09T17:50:23Z

@NickNaso The windows targets fail with nasty errors. Can you have a look ?

NickNaso · 2017-10-21T02:09:40Z

@agathver I have to do others little modifications. Can I add me to the contributors?

defunctzombie · 2017-10-21T02:16:35Z

package.json

@@ -29,7 +29,7 @@
    "install": "node-pre-gyp install --fallback-to-build"
  },
  "dependencies": {
-    "nan": "2.6.2",
+    "node-addon-api": "git+https://github.com/nodejs/node-addon-api.git",


Do not merge this. This is a dependency on a floating master branch and will lead to breaking changes. Please pick a specific version to depend on.

Yes you're right I want only understand where was the real problem if on the code or on ```node-addon-api`` version

defunctzombie · 2017-10-21T02:53:53Z

Is nan discontinued? Looking at the node-addon-api project, it seems to serve the exact same purpose as nan. What is the benefit of switching?

recrsn · 2017-10-21T02:59:25Z

@defunctzombie See this:

In short, this will bring "compile once, run in any node" versions of native addons, which will save dozens of "bcrypt does not work" issues on the release of a new node version.

NickNaso · 2017-10-21T11:26:29Z

Hi @agathver @defunctzombie
node-addon-api and nan are very different. NAN could be considered a thin layer build over the V8 API, so it's strong bounded with it. The main purpose of NAN is to guarantee the API compatibility across different version of Node.js.
node-addon-api is a C++ wapper of the new N-API https://nodejs.org/api/n-api.html it's completely JavaScript engine agnostic and it guarentees the API and ABI compatibility between different version of Node.js. So if you switch to a different version of Node.js you must not reinstall and maybe recompile the addon.
You could consider the idea to have two packages one with N-API and other with NAN as reported here:
https://nodejs.org/en/docs/guides/publishing-napi-modules/

defunctzombie · 2017-10-21T16:09:11Z

It is intended to insulate Addons from changes in the underlying JavaScript engine and allow modules compiled for one version to run on later versions of Node.js without recompilation.

This is a cute goal that I doubt will be achieved; maybe if they never introduce backwards breaking abi changes but with c++ that can be quite hard. In any case, it seems like this is the forward direction nodejs itself is promoting instead of nan. Thanks for doing the porting.

NickNaso · 2017-11-21T05:46:42Z

Hi @agathver @defunctzombie ,
today we have been released the new version of node-addon-api. I substituted it on package.json now all seems work fine.
What do you think if with this starting point we work together to publish a release of bcrypt that use N-API?
I mean to follow this guide https://nodejs.org/en/docs/guides/publishing-napi-modules/ obviously before that we need to do all work that you feel necessary to publish the addon.

ncb000gt · 2017-11-22T14:16:19Z

@NickNaso please feel free to add yourself to the list of contributors.

ncb000gt · 2017-11-22T16:35:19Z

src/bcrypt_node.cc

-        Nan::ThrowTypeError("3 arguments expected");
-        return;
+        throw Napi::TypeError::New(info.Env(), "3 arguments expected");
+        return info.Env().Undefined();


Why are these returns needed if you're throwing? I see them throughout, but thought they were unnecessary, no?

You're right it's not necessary if exceptions system is enable. I provided to remove all unuseful code.

… system is enabled

recrsn

Semver is nice but things break. Please pin the version numbers to prevent any accidental breakage ever.

recrsn · 2017-11-27T15:40:34Z

package.json

@@ -29,8 +29,9 @@
    "install": "node-pre-gyp install --fallback-to-build"
  },
  "dependencies": {
-    "nan": "2.6.2",
-    "node-pre-gyp": "0.6.36"
+    "bindings": "^1.3.0",


Please pin these dependencies.

Sorry I didn't understand. Do I need to add nan and node-pre-gyp ???

No. Just remove the caret. "^1.3.0" becomes "1.3.0" and so on.

Ok just done. Can you check?

NickNaso · 2017-12-07T21:45:14Z

Hi guys,
can you continue to review the code so we can publish the N-API version of bcrypt?

recrsn · 2017-12-08T05:40:25Z

@defunctzombie @ncb000gt Thoughts?

defunctzombie

LGTM we can iterate further as needed.

NickNaso · 2017-12-11T17:16:36Z

Hi guys,
what do you think if we tag the version as 1.0.3-napi and publish it throw this command:

npm publish --tag n-api

More explanation here:
To publish N-API version of a package alongside a non-N-API version

bcrypt for Node.js with Node addon api

NickNaso added 12 commits October 5, 2017 05:54

Add bindings and node-addon-api to dependencies

148db49

Added some boilerplate code to start migrating the module to a N-API

e122796

Porting of get_rounds -> GetRouds complete

c9ec383

Porting of compare_sync -> CompareSync complete

fead6b3

Porting of encrypt_sync -> EncryptSync complete

11378bc

Porting of compare -> Compare complete

e0d0b12

Porting of gen_salt -> GenSalt complete

92c538e

Portfing of method generate_salt_sync -> GenerateSaltSync complete

efae084

Porting of method encrypt -> Encrypt complete

b5c786e

Fix minor issue and test. All test successfully pass.

ed9ddef

Removed namespace

dbd032f

Removed some bad code

9c85bb0

recrsn previously requested changes Oct 9, 2017

View reviewed changes

recrsn requested a review from defunctzombie October 9, 2017 17:44

recrsn reviewed Oct 9, 2017

View reviewed changes

recrsn changed the title ~~bcrypt for Nofe.js with Node addon api~~ bcrypt for Node.js with Node addon api Oct 9, 2017

NickNaso added 11 commits October 12, 2017 08:10

Fixed typo issue on GenerateSaltSync method

b03a6e4

Update binding.gyp due to compilation problem on linux and windows

93ebfb3

Removed bindings

f4e4555

Added namespace Name on constructorAsyncWorker

bf07c28

Fix for compilation problem on windows platform

17d7d6d

Removed comments on GenerateSalt method

b19ba70

Added missing dependencies on binding.gyp

572f724

Update node-addon-api to the last version from the GitHub repo

62c7f29

Set engine support to 4 or greater

dca5076

Update travis config

48efe78

Update binding.gyp to fox problem on macOS with node version 4 and 5

7559cba

defunctzombie reviewed Oct 21, 2017

View reviewed changes

NickNaso mentioned this pull request Oct 21, 2017

N-API Support for node.bcrypt.js #513

Closed

NickNaso added 3 commits November 21, 2017 04:04

Update to new version (v1.1.0) of node-addon-api

72c1a3b

Add NodeJS 9 to CI

5467983

remove node 9 from appveyor

039b7ec

ncb000gt reviewed Nov 22, 2017

View reviewed changes

NickNaso added 2 commits November 22, 2017 23:00

Added on the contributors

f670313

Removed return info.Env().Undefined(); It's unnecessary if exceptions…

5cda155

… system is enabled

recrsn requested changes Nov 27, 2017

View reviewed changes

Updated dependencies on package.json as requested

30c8dc7

aruneshchandra mentioned this pull request Dec 5, 2017

Complete steps needed to get out of experimental nodejs/abi-stable-node#284

Closed

recrsn approved these changes Dec 8, 2017

View reviewed changes

defunctzombie approved these changes Dec 10, 2017

View reviewed changes

recrsn merged commit 3edea5f into kelektiv:napi Dec 12, 2017

recrsn mentioned this pull request Jan 13, 2018

ported bcrypt to new N-API #569

Closed

digitalinfinity mentioned this pull request Jan 26, 2018

N-API support for node-zopfli pierreinglebert/node-zopfli#92

Closed

fast-facts pushed a commit to fast-facts/node.bcrypt.js that referenced this pull request Jun 17, 2022

Merge pull request kelektiv#551 from NickNaso/node-addon-api

abcc647

bcrypt for Node.js with Node addon api

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bcrypt for Node.js with Node addon api #551

bcrypt for Node.js with Node addon api #551

NickNaso commented Oct 9, 2017 •

edited by recrsn

Loading

recrsn left a comment •

edited

Loading

recrsn Oct 9, 2017

NickNaso Oct 9, 2017

recrsn Oct 9, 2017

recrsn commented Oct 9, 2017

NickNaso commented Oct 21, 2017

defunctzombie Oct 21, 2017

NickNaso Oct 21, 2017

defunctzombie commented Oct 21, 2017

recrsn commented Oct 21, 2017

NickNaso commented Oct 21, 2017 •

edited

Loading

defunctzombie commented Oct 21, 2017

NickNaso commented Nov 21, 2017

ncb000gt commented Nov 22, 2017

ncb000gt Nov 22, 2017

NickNaso Nov 22, 2017 •

edited

Loading

recrsn left a comment

recrsn Nov 27, 2017

NickNaso Nov 27, 2017 •

edited

Loading

recrsn Nov 28, 2017

NickNaso Nov 28, 2017 •

edited

Loading

NickNaso commented Dec 7, 2017 •

edited

Loading

recrsn commented Dec 8, 2017

defunctzombie left a comment

NickNaso commented Dec 11, 2017

bcrypt for Node.js with Node addon api #551

bcrypt for Node.js with Node addon api #551

Conversation

NickNaso commented Oct 9, 2017 • edited by recrsn Loading

recrsn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recrsn commented Oct 9, 2017

NickNaso commented Oct 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defunctzombie commented Oct 21, 2017

recrsn commented Oct 21, 2017

NickNaso commented Oct 21, 2017 • edited Loading

defunctzombie commented Oct 21, 2017

NickNaso commented Nov 21, 2017

ncb000gt commented Nov 22, 2017

Choose a reason for hiding this comment

NickNaso Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

recrsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickNaso Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickNaso Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

NickNaso commented Dec 7, 2017 • edited Loading

recrsn commented Dec 8, 2017

defunctzombie left a comment

Choose a reason for hiding this comment

NickNaso commented Dec 11, 2017

NickNaso commented Oct 9, 2017 •

edited by recrsn

Loading

recrsn left a comment •

edited

Loading

NickNaso commented Oct 21, 2017 •

edited

Loading

NickNaso Nov 22, 2017 •

edited

Loading

NickNaso Nov 27, 2017 •

edited

Loading

NickNaso Nov 28, 2017 •

edited

Loading

NickNaso commented Dec 7, 2017 •

edited

Loading