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

fix: check and convert keypair to buffer #3

Merged
merged 1 commit into from
Jun 27, 2020

Conversation

aulneau
Copy link
Contributor

@aulneau aulneau commented Jun 27, 2020

These are the changes I made to get p2plex working in a next.js application (with these aliases via webpack). I have not tested to confirm that these changes work in a node.js env yet, but wanted to get this PR open! Related: #2
I also bumped some dependencies

Copy link
Owner

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

The buffer conversion code looks great, just have a minor concern about doing it in a getter rather than in the constructor. 😁

index.js Outdated
@@ -29,6 +29,20 @@ class P2Plex extends EventEmitter {
if (listenSelf) this.swarm.join(this.publicKey, { announce: true, lookup: false })
}

get keyPair () {
let { publicKey, secretKey } = this.opts.keyPair || noisePeer.keygen()
Copy link
Owner

Choose a reason for hiding this comment

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

I think it'd be good to move this initialization into the constructor, otherwise we could be calling noisePeer.keygen() ever time this.keypair is invoked (every time this.publicKey is invoked too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RangerMauve ah good call. Adjusted!

@aulneau aulneau requested a review from RangerMauve June 27, 2020 19:47
@RangerMauve RangerMauve merged commit af29fb5 into RangerMauve:master Jun 27, 2020
@RangerMauve
Copy link
Owner

Thank you for the fix! Hope it helps make stuff work on the web. 😁

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.

2 participants