Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

feat: support PeerInfo.create(IdJSON, cb) as well #61

Merged
merged 2 commits into from
Dec 14, 2017

Conversation

daviddias
Copy link
Member

No description provided.

@ghost ghost assigned daviddias Dec 14, 2017
@ghost ghost added the in progress label Dec 14, 2017

Creates a new PeerInfo instance from an existing PeerID.
Creates a new PeerInfo instance from an existing PeerId.
Copy link
Member

@victorb victorb Dec 14, 2017

Choose a reason for hiding this comment

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

Correction: Creates a new PeerInfo instance from an existing PeerId or creates a new PeerId for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

new PeerId doesn't do that. Users have always to resort to PeerInfo.create because creating a PeerId requires async calls (WebCrypto)

@@ -5,28 +5,29 @@ const chai = require('chai')
const dirtyChai = require('dirty-chai')
chai.use(dirtyChai)
const expect = chai.expect
const PeerId = require('peer-id')
const Id = require('peer-id')
Copy link
Member

Choose a reason for hiding this comment

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

Why the renames from PeerId -> Id and PeerInfo -> Info? Previous names were more clearer (without context)

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed there were multiple errors where PeerInfo.create({bits: 512}) appeared, which is a typo since only PeerId supports that option. My diagnostic is that it was hard to differentiate the two words.

@daviddias daviddias merged commit 5b8b160 into master Dec 14, 2017
@daviddias daviddias deleted the supportJSONId-as-well branch December 14, 2017 13:42
@ghost ghost removed the in progress label Dec 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants