hiredis-node icon indicating copy to clipboard operation
hiredis-node copied to clipboard

N-API Support for hiredis-node

Open aruneshchandra opened this issue 7 years ago • 15 comments

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.

hiredis-node 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 hiredis-node 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

aruneshchandra avatar Jun 07 '17 16:06 aruneshchandra

Hey, thanks for reaching out.

If N-API is the way forward, I appreciate that. But at the moment I am not a user of hiredis-node myself nor do I have much time to approach a full rewrite to this new API. If someone does the effort and provides a full implementation I would be happy to incorporate this here. It won't be me though.

badboy avatar Jun 07 '17 16:06 badboy

I completely understand your point of view! Will you be willing to talk to us during our Weekly WG meeting which happens every Thursday at 1.30 Eastern / 10.30 Pacific US time, or one on one over the phone about this ?

aruneshchandra avatar Jun 07 '17 17:06 aruneshchandra

Not before July, but for that I'll put it on my schedule and let you know a bit in advance.

badboy avatar Jun 07 '17 17:06 badboy

No problem - we can wait! LMK when we can get some time with you to discuss this a little bit more.

aruneshchandra avatar Jun 07 '17 17:06 aruneshchandra

@badboy do you think you have time to talk about this ?

aruneshchandra avatar Jul 13 '17 19:07 aruneshchandra

Yes! Thanks for reminding me again. I see the next meeting is scheduled for next week, which won't work for me though. How should we get in contact?

badboy avatar Jul 13 '17 21:07 badboy

When are you available @mhdawson and I can set up a separate call with you on this.

aruneshchandra avatar Jul 14 '17 16:07 aruneshchandra

@aruneshchandra Sorry for letting this slip. Is there a meeting this week? I would have time to join in.

badboy avatar Aug 01 '17 16:08 badboy

Yes there is a meeting this week on Thursday at 1.30 Eastern / 10.30 Pacific US time. Here's the hangout link https://plus.google.com/u/0/events/c0eevtrlajniu7h8cjrdk0f56c8?authkey=COH04YCalJS8Ug

Look forward to talking to you :)

aruneshchandra avatar Aug 01 '17 16:08 aruneshchandra

Here is the link to the repo, the readme has instructions on how to use the migration tool:

https://github.com/nodejs/node-addon-api

mhdawson avatar Aug 03 '17 17:08 mhdawson

Ok, I took a first stab using the conversion tool and have to say I hoped for a bit more. Some notes:

  • The conversion tool is a mere text replacement, editing the code to refer to non-existing variables here and there. That should be marked more clearly.
  • I'm missing a document providing a clearer guide on replacements of the old API. I do understand the conversion tool can't handle everything, but without knowledge about the new APIs it is hard to track down what code to add (the examples are a good start, but don't cover a whole lot)
  • The node-addon-api seems to track node.js master and didn't work with my installed node v8.2.1. I had to find that in an issue, it is not documented anywhere.

I don't have any compilable code so far, but will take another stab at it in a couple of days.

badboy avatar Aug 04 '17 13:08 badboy

//cc: @sampsongao @jasongin

aruneshchandra avatar Aug 04 '17 16:08 aruneshchandra

Hi everyone, I worked on porting hiredis to N-API. You can find my work here: https://github.com/NickNaso/hiredis-node/tree/napi If you want experiment with N-API you can create a branch and call it "napi" so I can execute the PR on it. After that we can iterate to over it and at the end publish a tagged version of hiredis like reported here: https://nodejs.org/en/docs/guides/publishing-napi-modules/

NickNaso avatar Apr 04 '18 23:04 NickNaso

Hi everyone,

I don't think hiredis-node (or hiredis for that matter) presently have any active maintainers, but I created the napi branch as requested.

I'm not a java/node dev but am happy to help with the process where I can.

Cheers! Mike

michael-grunder avatar Apr 07 '18 23:04 michael-grunder

Hi, I'll apply for the maintainer role:)

karawitan avatar Oct 13 '19 15:10 karawitan