-
Notifications
You must be signed in to change notification settings - Fork 44
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.
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
Conversation
fsdiogo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/index.js
Outdated
| } | ||
|
|
||
| exports = module.exports = PeerId | ||
| exports = module.exports = withIs(PeerId, { className: 'PeerId', symbolName: '@libp2p/js-peer-id/PeerId' }) |
There was a problem hiding this comment.
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:
Line 157 in 6513a02
| callback(null, new PeerId(digest, privKey)) |
Line 162 in 6513a02
| return new PeerId(mh.fromHexString(str)) |
Line 166 in 6513a02
| return new PeerId(buf) |
Line 170 in 6513a02
| return new PeerId(mh.fromB58String(str)) |
Line 199 in 6513a02
| callback(null, new PeerId(digest, null, pubKey)) |
Line 231 in 6513a02
| callback(null, new PeerId(digest, privKey, privKey.public)) |
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
pgte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
As `instance of` fails using different versions of a module, it was changed to use `class-is`. Needs: - [x] [js-peer-id#84](libp2p/js-peer-id#84) - [x] [interface-datastore#24](ipfs/interface-datastore#24) Fixes #1615
The
instance offails when using different versions of a module. This way, we end up with problems using this in other places, such as js-ipfs/core/ipns/publisher.js#L54.In the context of js-ipfs/issues/1615. This is not a problem now, as it is only occurring with
interface-datastorekey. However, it may occur in a future release of this module.