-
Notifications
You must be signed in to change notification settings - Fork 48
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
chore: fix types #321
chore: fix types #321
Conversation
achingbrain
commented
Apr 16, 2021
- Remove bignumber.js in favour of BigInt
- Use ipld-block in favour of the old ipfs-core-types
- Use blockstore from ipfs-repo instead of the old ipfs-core-types
- Remove bignumber.js in favour of BigInt - Use ipld-block in favour of the old ipfs-core-types - Use blockstore from ipfs-repo instead of the old ipfs-core-types
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
Couple of nits in review
package.json
Outdated
"cids": "^1.1.6", | ||
"debug": "^4.2.0", | ||
"ipfs-core-types": "^0.3.1", | ||
"ipfs-core-types": "next", |
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.
we need to get rid of these circular dependencies
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.
I don't think this is even required now, since we can pull the Blockstore
type from ipfs-repo
it's only in the tests, and even then it was only being used to pull types for the repo, which are in ipfs-repo
now so it can go.
@@ -41,15 +34,12 @@ class WantListEntry { | |||
} | |||
|
|||
/** | |||
* @param {API} other | |||
* @returns {boolean} | |||
* @param {any} other |
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.
* @param {any} other | |
* @param {unknown} other |
We are using unknown for such scenarios in libp2p. Perhaps better?
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.
I don't think you can access properties of a variable of type unknown
?
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.
We do it here https://github.com/libp2p/js-libp2p/blob/master/src/record/peer-record/index.js#L68
does not seem problematic
src/utils/index.js
Outdated
@@ -6,7 +6,7 @@ const uint8ArrayEquals = require('uint8arrays/equals') | |||
/** | |||
* Creates a logger for the given subsystem | |||
* | |||
* @param {import('peer-id')} [id] | |||
* @param {any} [id] |
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.
can this be other than 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.
Oops, left that in by accident - I was toying with the idea of using CIDs instead of PeerIds