search

mscdex / node-xxhash

An xxhash binding for node.js
Other
193 stars 28 forks source link

Node 12, xxhash v0.6.5, updated CI configs #30

Closed ben-page closed 5 years ago

ben-page commented 5 years ago

This fixes support for NodeJS 12, updated the xxhash dependency to v0.6.5, and fixes the broken CI configs.

Also, sorry about the noise on the previous PRs. I wanted to separate these three items into separate PRs. There was just no good way to do that until the CI scripts are fixed.

The current travis config always fails for travis and doesn't build against recent versions of NodeJS. So I needed the improved CI configs to prove the Node 12 support worked. So I decided that putting them into one PR with three commits is the best choice. Let me know if you prefer something else.

ben-page commented 5 years ago

@mscdex, I'm hoping you can pull this soon. The current release does not work with Node.js 12

mscdex commented 5 years ago

There's a lot of extra changes and additions here that I don't really agree with. I think the changes should strictly be related to upgrading the code base to use nan-related functionality where possible and updating the upstream dependency (still as separate commits).

ben-page commented 5 years ago

My intention definitely wasn't to make any unnecessary changes or addition. I'd like this to be very easy to pull. Would you mind clarifying what you object to?

I'll go over each commit to justify the changes that I made.

upgrades xxhash to v0.6.5 (d59891f) The only changes I made were literally extracting the official xxHash-0.6.5.zip file into the deps folder and changing a preprocess directive to be compatible with xxhash code.

fixes NodeJS 12 compatibility (1604274) I'm not experienced with NAN, so I literally only made changes remove the errors and warnings thrown when building against Node.js 12. Assuming that isn't the problem. Maybe you are referring to the formatting change to the package.json file? That was done by npm i nan@lastest. I did not edit the file manually. Also, I included a .gitignore file that ensures neither the node_modules or build folders are include in git or npm when publish. I thought this was a reasonable change, but I will happily remove it if you would like.

updated CI based on the recommendations (a8c656c) This updates the travis config (which is currently broken and out of date) and the appveyor config (which is out of date). I started with the recommended configs from https://github.com/nodejs/node-addon-api/. The only changes I made was to add Node.js 12 (they don't include it yet) and remove chakracore. These configs do not build against Node.js 0.10 and 0.12. So, I guess this is implicitly dropping support for those versions. If you think it's important to support them, I will add the build configurations and ensure they work.

mscdex commented 5 years ago

I'm not against dropping v0.10 and v0.12 support.

As far as the CI configs go, you can probably use the configs from mmmagic as a guide since I recently updated them.

ben-page commented 5 years ago

OK, I imported the CI configs from mmagic. No changes were needed.

ben-page commented 5 years ago

Removed .gitignore and formatting change to package.json

asilvas commented 5 years ago

Would be nice to move this PR forward.

imrehg commented 5 years ago

Would love to have this PR in, getting ready for Node 12 in a bunch of projects which use this :)

asilvas commented 5 years ago

@ben-page can you make the last requested changes? Not sure if that is what is holding this up..

alexandrebodin commented 5 years ago

Hi @mscdex @ben-page, any one of you willing to finish this PR ? and move forward ? Thanks !

mscdex commented 5 years ago

Landed in 0609e6235e7fdc49b83b2754c779cb42858f668b, bd134d81d584edc83bcba83032e75e91d7d23cdd, and 6fa15b74508bd8fece248a6c4a0fe7e1229ceefc. Thanks!