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

Adds support for Node version >=12 #396

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

agbilotia1998
Copy link
Contributor

Fixes #346

@agbilotia1998
Copy link
Contributor Author

@simonweniger please review, adding this comment in case auto review request is not created. Thanks :)

@simonweniger
Copy link
Member

@troggy can you review this ?

Copy link

@TheReturnOfJan TheReturnOfJan left a comment

Choose a reason for hiding this comment

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

Looks good!

Review steps:

  1. I checkout latest master, ran yarn with node 11 and it worked fine.
  2. I switched to node 12, and got an error after running yarn.
  3. I checked-out this branch.
  4. Ran yarn and yarn test on both node 11 and node 12, both working fine.

@johannbarbie johannbarbie merged commit 6a0d417 into leapdao:master Jan 13, 2020
@agbilotia1998
Copy link
Contributor Author

@simonweniger I've submitted this as work on Gitcoin, please have a look. Thanks :)

@simonweniger
Copy link
Member

@agbilotia1998 payout done, thank you for the contribution. We would love to have you as a contributor. So pleas check other available bountys at leapdao.org/earn. You also will recive LeapTokens as a reputation payment, so make sure you aktivate them in your wallet. This is the Leap Token adress: 0x78230e69d6e6449db1e11904e0bd81c018454d7a

@agbilotia1998
Copy link
Contributor Author

I've added Leap Token to my account, sure I'll have a look at other bounties as well. Thanks :)

@troggy
Copy link
Member

troggy commented Jan 18, 2020

I've found one side effect for using leap-node with node 12.

leap-node uses keccak package via ethereumjs-util package. This package supports native builds (via node-gyp) and falls back to pure JS implementation if it fails to compile native build on install. What I've noticed is that keccak native build always fails for node 12, so pure js implementation will always be used. I'm not sure what are implications of this, probably only possible performance degradation?

Related issue: cryptocoinjs/keccak#10 (resolved in keccak@2.1.0, but latest ethereumjs-util still uses older version)

/cc @TheReturnOfJan

@troggy
Copy link
Member

troggy commented Jan 18, 2020

How to check:

if your installation uses js implementation of keccak you will see this on startup: Keccak bindings are not compiled. Pure JS implementation will be used.

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.

support "yarn global add leap-node" with node -v >= 12
5 participants