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

Move over to N-API from NAN #71

Merged
merged 13 commits into from
Sep 26, 2019
Merged

Move over to N-API from NAN #71

merged 13 commits into from
Sep 26, 2019

Conversation

anandsuresh
Copy link
Owner

No description provided.

Copy link
Contributor

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

Hi @anandsuresh it seems good to me. It's a good starting point to next release

@anandsuresh
Copy link
Owner Author

@NickNaso What would be a good next step? Should we make a PR against the NAPI repo linking this branch to the list of NAPI modules?

@NickNaso
Copy link
Contributor

NickNaso commented Jun 1, 2018

Hi @anandsuresh I linked the issue BAPI Support with an issue on the abi-stable-node repo https://github.com/nodejs/abi-stable-node/issues/246 . In this issue we report our porting activities and if you want fell free to report your experience with N-API.
For the sse4_crc32 module we can publish a tagged version of the module as reported here in this guide https://nodejs.org/en/docs/guides/publishing-napi-modules/ this means that people can start using the N-API version of the module today if they install like reported below:
npm install sse4_crc32@n-api
After some time we can consider to switch to N-API version.
if you will publish the module please ping me so I will provide to spread the news of the porting.

@zbjornson
Copy link
Contributor

@anandsuresh it looks like this is based on an old commit, based on the diff. Are you still planning to move forward with this? There's some code cleanup (around the native build, quite a bit of code that can be removed) that I'd love to contribute, but don't want to mess up your N-API work. On the other hand, you might have an easier time converting to N-API with my proposed cleanup.

@anandsuresh
Copy link
Owner Author

anandsuresh commented Nov 9, 2018 via email

@ivan
Copy link
Contributor

ivan commented Jun 1, 2019

I have updated this PR over at https://github.com/ludios/sse4_crc32/commits/master and I am testing it, feel free to take my commits.

@stephenplusplus
Copy link

@anandsuresh Hello! Any thoughts on how we should proceed with this migration?

@stephenplusplus
Copy link

@ivan would you be willing to send your changes as a PR here? Whatever we can do to make it easier for @anandsuresh to merge and release 🙏

@anandsuresh
Copy link
Owner Author

anandsuresh commented Sep 17, 2019 via email

@anandsuresh anandsuresh merged commit 7d8526a into master Sep 26, 2019
@ivan
Copy link
Contributor

ivan commented Sep 26, 2019

@anandsuresh FYI I think there's a UAF here that I fixed with ludios@d39bdd7

@anandsuresh
Copy link
Owner Author

Thanks @NickNaso for getting this started, and to @ivan for the fixes. With v6, the package is now available on NPM as well as GitHub Package Registry! Apologies for delays at my end.

@anandsuresh anandsuresh deleted the napi branch September 26, 2019 05:23
@anandsuresh anandsuresh mentioned this pull request Sep 26, 2019
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

Successfully merging this pull request may close these issues.

5 participants