Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

N-API support #69

Closed
NickNaso opened this issue May 16, 2018 · 9 comments
Closed

N-API support #69

NickNaso opened this issue May 16, 2018 · 9 comments

Comments

@NickNaso
Copy link
Contributor

Node 8 introduced feature called N-API which is aimed at reducing maintenance cost for node native addons.
sse4_crc32 is a popular module, and in order to help the Node.js community make the important transition to N-API, we are hoping you will be able to work with us in order to migrate sse4_crc32 to N-API. Your support and feedback is important in making this effort a success.

We (N-API team) have built a preliminary port of sse4_crc32 on top of N-API as part of our effort to understand the API surface used by native modules. We used this port and the port of some other modules to determine which areas to focus on converting to N-API.
This port has all of the NAN code removed and replaced by N-API equivalents. While it passes all the unit tests, it might need a bit more refinement before getting merged in.
Please take a look if you're interested. I will be happy to answer questions about the port, point to documentation/code, etc. I'm also happy to open a PR if you're happy with the port in it's current shape.

If you don't want to take a dependency on a new feature, we understand, and a good option might be to maintain a branch that uses N-API, that is kept mostly up to date with development occurring in sse4_crc32/master. We have some precedence to follow with node.bcrypt.js which recently published a N-API version of their module. Of course we'd appreciate feedback about anything we can do to improve the development or migration experience.

You can find my work here: https://github.com/NickNaso/sse4_crc32/tree/napi

I will be happy to contribute and help in any way.

@anandsuresh
Copy link
Owner

This is awesome! I've been toying with the idea of porting this to the N-API module for a while, but haven't had the time to follow through.

Have taken a peek at the code you have and it seems good to me. Given the nature of the change, I'm thinking this should go out as a major release (v6) while keeping v5 as a NAN module for backwards compatibility.

Feel free to shoot me a PR. Also let me know if I can help this effort in any other ways.

@NickNaso
Copy link
Contributor Author

NickNaso commented May 17, 2018

Hi @anandsuresh,
if you agree and 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 sse4_crc32 like reported here: https://nodejs.org/en/docs/guides/publishing-napi-modules/
In this way an end user can install and test the N-API version of the module that isn't the official version.
Obviously when you go out the major release (v6) you can definitely switch to N-API.
What do you think?

@anandsuresh
Copy link
Owner

Sounds good to me. New branch napi created.

@NickNaso
Copy link
Contributor Author

OK Just did the PR here #70

@anandsuresh
Copy link
Owner

Sweet. I have been meaning to make changes to the interface to the library, mainly adding a stream-based interface and updating some of the existing ones. Maybe this would be a good place to start. I worked on some of the changes this weekend. Still need to get to the tests and update the benchmarks. Will get to it sometime this week. Does that work for you?

@NickNaso
Copy link
Contributor Author

It sounds good. Keep me updated about that. I'm available to help on this.

@anandsuresh
Copy link
Owner

Hey Nick, Got the changes in and tested.

@NickNaso
Copy link
Contributor Author

Hi @anandsuresh I looked at code and it seems good to me. It's a good start for the next release.

@anandsuresh
Copy link
Owner

Merged in #71

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

No branches or pull requests

2 participants