Skip to content

feat: add class-is module #84

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.

Sign up for GitHub

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

Merged
pgte merged 2 commits into from
Oct 18, 2018
Merged

feat: add class-is module #84

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6513a02
feat: add class-is module
vasco-santos Oct 4, 2018
2e5e666
fix: add peerIdWithIs to the API functions using the instance
vasco-santos Oct 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
},
"dependencies": {
"async": "^2.6.1",
"class-is": "^1.1.0",
"libp2p-crypto": "~0.13.0",
"lodash": "^4.17.10",
"multihashes": "~0.4.13"
Expand Down
3 changes: 2 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const mh = require('multihashes')
const crypto = require('libp2p-crypto')
const assert = require('assert')
const waterfall = require('async/waterfall')
const withIs = require('class-is')

class PeerId {
constructor (id, privKey, pubKey) {
Expand Down Expand Up @@ -132,7 +133,7 @@ class PeerId {
}
}

exports = module.exports = PeerId
exports = module.exports = withIs(PeerId, { className: 'PeerId', symbolName: '@libp2p/js-peer-id/PeerId' })
Copy link
Member

@alanshaw alanshaw Oct 15, 2018

Choose a reason for hiding this comment

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

There are a number of occasions in this file where we're accessing the non-wrapped PeerId class:

js-peer-id/src/index.js

Line 157 in 6513a02

callback(null, new PeerId(digest, privKey))

js-peer-id/src/index.js

Line 162 in 6513a02

return new PeerId(mh.fromHexString(str))

js-peer-id/src/index.js

Line 166 in 6513a02

return new PeerId(buf)

js-peer-id/src/index.js

Line 170 in 6513a02

return new PeerId(mh.fromB58String(str))

js-peer-id/src/index.js

Line 199 in 6513a02

callback(null, new PeerId(digest, null, pubKey))

js-peer-id/src/index.js

Line 231 in 6513a02

callback(null, new PeerId(digest, privKey, privKey.public))

js-peer-id/src/index.js

Line 282 in 6513a02

callback(null, new PeerId(id, priv, pub))

These all need to be changed otherwise they create PeerId objects that don't have the '@libp2p/js-peer-id/PeerId' symbol and so aren't considered to be instances of PeerId when used with PeerId.isPeerId.

Copy link
Member Author

@vasco-santos vasco-santos Oct 17, 2018

Choose a reason for hiding this comment

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

Yes! Thanks for noticing that.

I added PeerIdWithIs to all of them. However, I feel that there might be a better approach here. What do you think?

Copy link
Member

@alanshaw alanshaw Oct 17, 2018

Choose a reason for hiding this comment

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

Not really, js-cid uses _CID https://github.com/ipld/js-cid/blob/6d249ab8a3cc4000a1db0467f23046650453049a/src/index.js#L243


// generation
exports.create = function (opts, callback) {
Expand Down