Skip to content
This repository has been archived by the owner on Nov 21, 2019. It is now read-only.

N-API Support for hiredis-node #134

Open
aruneshchandra opened this issue Jun 7, 2017 · 15 comments
Open

N-API Support for hiredis-node #134

aruneshchandra opened this issue Jun 7, 2017 · 15 comments

Comments

@aruneshchandra
Copy link

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

@badboy
Copy link
Contributor

badboy commented Jun 7, 2017

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.

@aruneshchandra
Copy link
Author

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 ?

@badboy
Copy link
Contributor

badboy commented Jun 7, 2017

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

@aruneshchandra
Copy link
Author

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

@aruneshchandra
Copy link
Author

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

@badboy
Copy link
Contributor

badboy commented Jul 13, 2017

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?

@aruneshchandra
Copy link
Author

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

@badboy
Copy link
Contributor

badboy commented Aug 1, 2017

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

@aruneshchandra
Copy link
Author

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 :)

@mhdawson
Copy link

mhdawson commented Aug 3, 2017

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

@badboy
Copy link
Contributor

badboy commented Aug 4, 2017

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.

@aruneshchandra
Copy link
Author

//cc: @sampsongao @jasongin

@NickNaso
Copy link

NickNaso commented Apr 4, 2018

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/

@michael-grunder
Copy link
Collaborator

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

@karawitan
Copy link

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

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

No branches or pull requests

6 participants