Migration to N-API #189

MrKashlakov · 2018-11-08T08:04:40Z

Hello!
I just did migration of this module to N-API. Please review this PR.
All tests passed!

gabrielschulhof

Excellent! I'm glad you were able to address the test failures after our N-API workshop 👍

gabrielschulhof · 2018-11-09T16:18:52Z

package-lock.json

@@ -0,0 +1,18 @@
+{


@MrKashlakov I don't believe this file should be part of the commit. You should probably remove it.

gabrielschulhof · 2018-11-09T16:20:05Z

src/binding.cc

@@ -15,7 +15,8 @@
 */

 #include "iconv.h"
-#include "nan.h"
+#include "napi.h"
+#include "uv.h"


@MrKashlakov why do you need "uv.h"?

gabrielschulhof · 2018-11-09T16:37:20Z

src/binding.cc

@@ -15,7 +15,8 @@
 */

 #include "iconv.h"
-#include "nan.h"
+#include "napi.h"
+#include "uv.h"
 #include "node_buffer.h"


@MrKashlakov I believe you can remove this include, because node-addon-api provides access to node::Buffer via N-API.

gabrielschulhof · 2018-11-09T16:38:18Z

src/binding.cc

-             Nan::New<FunctionTemplate>(Convert)->GetFunction());
+    Iconv::Init(env);
+    exports.Set(Napi::String::New(env, "make"),
+             Napi::Function::New(env, Make));


This line should be indented so the Napi::... is in the same column as the line above.

gabrielschulhof · 2018-11-09T16:38:31Z

src/binding.cc

+    exports.Set(Napi::String::New(env, "make"),
+             Napi::Function::New(env, Make));
+    exports.Set(Napi::String::New(env, "convert"),
+             Napi::Function::New(env, Convert));


Same here wrt. indentation.

gabrielschulhof · 2018-11-09T16:39:46Z

src/binding.cc


+using namespace Napi;


You don't actually need this line, because the code uses Napi::... explicitly everywhere – which is good.

gabrielschulhof · 2018-11-09T16:42:33Z

src/binding.cc

  }

  // Forbid implicit copying.
  Iconv(const Iconv&);
  void operator=(const Iconv&);
+


This new empty line is not needed.

gabrielschulhof · 2018-11-09T16:50:45Z

binding.gyp

@@ -6,7 +6,24 @@
  'targets': [
    {
      'target_name': 'iconv',
-      'include_dirs': ['<!(node -e "require(\'nan\')")'],
+      'cflags!': [ '-fno-exceptions' ],


The original version of node-iconv made sure there were no CPP exceptions. Thus, we should maintain that choice. This means that cflags! needs to be changed to cflags.

gabrielschulhof · 2018-11-09T16:51:04Z

binding.gyp

+      'cflags!': [ '-fno-exceptions' ],
+      'cflags_cc!': [ '-fno-exceptions' ],
+      'xcode_settings': { 
+        'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',


... and this needs to be changed to 'NO'.

gabrielschulhof · 2018-11-09T17:31:12Z

binding.gyp

@@ -6,7 +6,24 @@
  'targets': [
    {
      'target_name': 'iconv',
-      'include_dirs': ['<!(node -e "require(\'nan\')")'],
+      'cflags!': [ '-fno-exceptions' ],
+      'cflags_cc!': [ '-fno-exceptions' ],


... and cflags_cc! needs to be changed to cflags_cc.

gabrielschulhof · 2018-11-09T17:32:00Z

package.json

@@ -18,7 +18,7 @@
  "license": "ISC",
  "readmeFilename": "README.md",
  "dependencies": {
-    "nan": "^2.11.1",
+    "node-addon-api": "1.6.0",


This should be ^1.6.0 so that, when node-addon-api@1.7.0 comes out this package will start using it.

bnoordhuis

Thanks, left some comments but mostly LGTM. I'll second Gabriel's comments.

bnoordhuis · 2018-11-10T11:46:20Z

binding.gyp

+      'cflags_cc!': [ '-fno-exceptions' ],
+      'xcode_settings': { 
+        'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
+        'CLANG_CXX_LIBRARY': 'libc++',


Can you remove this? It's incompatible with Node.js v6.x. node-iconv can just inherit it from node's common.gypi.

bnoordhuis · 2018-11-10T11:47:41Z

binding.gyp

+        'support'
+      ],
+      'dependencies': [
+          "<!(node -p \"require('node-addon-api').gyp\")"],


Can you use single quotes for consistency?

bnoordhuis · 2018-11-10T11:48:11Z

binding.gyp

+      },
+      'msvs_settings': {
+        'VCCLCompilerTool': { 'ExceptionHandling': 1 },
+      },


This block should be removed.

bnoordhuis · 2018-11-10T11:49:22Z

src/binding.cc

+    Napi::ObjectWrap<Iconv>(info) {
+      conv_ = info[0].As<Napi::External<iconv_t>>().Data();
+    }
+  iconv_t conv_;


Inconsistent indentation, and can you move conv_ back to its old place again? Cheers.

bnoordhuis · 2018-11-10T11:55:26Z

src/binding.cc

+    size_t input_size = info[3].IsNumber() ? info[3].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);
+    char* output_buf = info[4].As<Napi::Buffer<char>>().Data();
+    size_t output_start = info[5].IsNumber() ? info[5].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);
+    size_t output_size = info[6].IsNumber() ? info[6].As<Napi::Number>().Uint32Value() : static_cast<uint32_t>(0);


Why the ternaries? The JS layer always passes up numbers.

mhdawson · 2019-01-28T18:21:13Z

@bnoordhuis is this something you'd be able to review/consider landing?

mhdawson · 2019-01-28T18:22:58Z

Just confirming you are waiting on responses to your comments and once they are addressed it might move forward as we are are talking about whether we need to find somebody else to take over the pr.

@MrKashlakov are you going to be able to get back to this in order to address the comments.

bnoordhuis · 2019-01-28T23:22:51Z

Just confirming you are waiting on responses to your comments and once they are addressed it might move forward

That's correct. This PR also needs a rebase now.

NickNaso · 2019-02-04T22:55:32Z

Hi @MrKashlakov I hope that you have the time to finish this work, but in case if you are busy what do you think if I help you to accomplish at tasks requested in this PR?

MrKashlakov added 2 commits November 7, 2018 17:07

Migration to n-api

777123e

Migration to n-api: safe convertion to uint32_t

1fb6ea2

gabrielschulhof reviewed Nov 9, 2018

View reviewed changes

src/binding.cc

}

// Forbid implicit copying.

Iconv(const Iconv&);

void operator=(const Iconv&);

Copy link

gabrielschulhof Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new empty line is not needed.

gabrielschulhof reviewed Nov 9, 2018

View reviewed changes

bnoordhuis reviewed Nov 10, 2018

View reviewed changes

targos mentioned this pull request Dec 15, 2018

Check if citgm is clean on Node master nodejs/citgm#660

Closed

digitalinfinity mentioned this pull request Dec 20, 2018

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

Closed

NickNaso mentioned this pull request Feb 24, 2019

Some fixes after review MrKashlakov/node-iconv#1

Open

NickNaso mentioned this pull request Mar 4, 2019

First step on N-API porting #196

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migration to N-API #189

Migration to N-API #189

MrKashlakov commented Nov 8, 2018

gabrielschulhof left a comment

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018 •

edited

Loading

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018 •

edited

Loading

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018

gabrielschulhof Nov 9, 2018

bnoordhuis left a comment

bnoordhuis Nov 10, 2018

bnoordhuis Nov 10, 2018

bnoordhuis Nov 10, 2018

bnoordhuis Nov 10, 2018

bnoordhuis Nov 10, 2018

mhdawson commented Jan 28, 2019

mhdawson commented Jan 28, 2019

bnoordhuis commented Jan 28, 2019

NickNaso commented Feb 4, 2019

Migration to N-API #189

Are you sure you want to change the base?

Migration to N-API #189

Conversation

MrKashlakov commented Nov 8, 2018

gabrielschulhof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof Nov 9, 2018 • 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

Choose a reason for hiding this comment

gabrielschulhof Nov 9, 2018 • 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

bnoordhuis left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jan 28, 2019

mhdawson commented Jan 28, 2019

bnoordhuis commented Jan 28, 2019

NickNaso commented Feb 4, 2019

gabrielschulhof Nov 9, 2018 •

edited

Loading

gabrielschulhof Nov 9, 2018 •

edited

Loading