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

There a lot of thing has been update by krisklosterman #128

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

jeong760
Copy link

Dears,

Could you approve PR request? krisklosterman has been update and changed many things.

Regards,
John Ahn

@tofra
Copy link

tofra commented Aug 10, 2017

I wouldn't merge this one, you are changing the package. Now zone117x/node-multi-hashing.git is used, and you are changing it to one of your own..

@jcreyesb
Copy link

Hi, can you add M7 algo?

@HashUnlimited
Copy link
Collaborator

this PR contains a lot of updates which actually make sense and I wanted to do anyways. so we gonna merge this and revert the changes with issues, that way is simpler than cherry-picking all the useful stuff.

@MueR
Copy link

MueR commented May 30, 2018

You'll probably want to change the bignum library too then. I recommend https://github.com/MikeMcl/bignumber.js, that's the one I'm using on my version. Excellent support from the author too.

@HashUnlimited
Copy link
Collaborator

@MueR I was indeed thinking about that. The one you are suggesting might be lightweight but does not cover the necessary precision for all use cases.
The only disadvantage of the npm version is the node compatibility (>=4.3 I think). We have to ask ourselves if we REALLY need to go down that far as in the meanwhile the oldest LTS is Node 6. My latest changes in the NOMP init.js require 6 btw but they could be easily adapted. In my personal opinion I would prefer to go for Node 6 (in dev first of course) when doing the overhaul to get rid of tons of deprecation warnings everywhere in my IDE. Of course open for discussion, @zone117x what do you think? After the overhaul and some good testing, I would suggest to rename and tag the current master branch, bump the version on dev, rename it and make it default. Create a new dev of course.

@MueR
Copy link

MueR commented May 30, 2018

I've converted everything to run on the 8.11 LTS version, using full ES2017 features. Just about everything is in javascript modules now. I've not found any problems with bignumber.js so far, but maybe I'm missing something?

@zone117x
Copy link
Owner

If I had the time I would aim to support latest LTS and latest stable. Just don't have the time to implement or vet the changes. Would really appreciate it if @MueR did a pull request for the updates and @HashUnlimited could help review them.
I do agree about clearing tagging and branching since this repo has had no breaking changes for so long.

@MueR
Copy link

MueR commented May 30, 2018

I'll have to see if I can find some time this week @zone117x and @HashUnlimited. I've done a bit more than just rewrite everything into modules, I've also added some proprietary things that I'm not willing to share readily. But I'll see about filtering some of the custom work out soonish.

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.

7 participants