-
Notifications
You must be signed in to change notification settings - Fork 445
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
feat: convert to typescript #1172
Conversation
BREAKING CHANGE: types are no longer hand crafted, this module is now ESM only
2022-03-22 conversation for someone reviewing this code:
|
import errCode from 'err-code' | ||
import type { RecursivePartial } from '@libp2p/interfaces' | ||
|
||
const DefaultConfig: Partial<Libp2pInit> = { |
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.
How come you put Partial here?
Shouldn't we enforce that whoever adds a new module must put defaults here? Maybe even create some type which would make everything mandatory here 😄
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.
Without Partial
is becomes necessary to define things things like .peerId
and .datastore
that aren't simple values.
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.
const DefaultConfig: Partial<Libp2pInit> = { | |
const DefaultConfig: Required<Omit<Libp2pInit, "peer" | "datastore">> & Libp2pInit = { |
would probably do the trick but I don't have a strong preference, it might end up being too hard to maintain what to keep optional.
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.
Yeah, it starts getting a bit fiddly like this, maybe let's look at it again later.
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.
Update LGTM. Some small comments regarding code snippets in docs. We should consider adding something like https://github.com/bbc/typescript-docs-verifier to the CI pipeline.
|
||
const node = await createLibp2p({ | ||
transports: [ | ||
TCP, |
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 think this doesn't work anymore, same below where classes are used instead of instances.
import { Mplex } from '@libp2p/mplex' | ||
import { Noise } from '@chainsafe/libp2p-noise' | ||
|
||
const { FaultTolerance } from 'libp2p/src/transport-manager') |
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.
invalid syntax
@@ -22,8 +22,8 @@ | |||
Sometimes you may need to wrap an existing duplex stream in order to perform incoming and outgoing [transforms](#transform) on data. This type of wrapping is commonly used in stream encryption/decryption. Using [it-pair][it-pair] and [it-pipe][it-pipe], we can do this rather easily, given an existing [duplex iterable](#duplex). | |||
|
|||
```js | |||
const duplexPair = require('it-pair/duplex') | |||
const pipe = require('it-pipe') | |||
const duplexPair from 'it-pair/duplex') |
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.
syntax error
@@ -80,7 +82,7 @@ libp2p.dialProtocol(peerInfo, '/echo/1.0.0', (err, conn) => { | |||
|
|||
**After** | |||
```js | |||
const pipe = require('it-pipe') | |||
const pipe from 'it-pipe') |
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.
syntax
Addresses PR comments from #1172 - fixes syntax of examples in docs, adds the transport manager to the exports map and renames fault tolerance enum for consistency.
@wemeetagain thanks for your review - I've addressed the points in #1182 bbc/typescript-docs-verifier - this looks really useful, I didn't know it existed. Would you like to give adding it to CI a go? |
Ok, I'll open a PR |
Addresses PR comments from #1172 - fixes syntax of examples in docs, adds the transport manager to the exports map and renames fault tolerance enum for consistency.
Converts this module to typescript.
libp2p-tcp
to@libp2p/tcp
transport
->transports
,connEncryption
->connectionEncryption
. In general where we pass multiple things, the key is now plural, e.g.streamMuxer
->streamMuxers
,contentRouting
->contentRouters
, etc. Where we are configuring a singleton the config key is singular, e.g.connProtector
->connectionProtector
etc.modules
config key have been moved to the rootconfig
config key have been moved to the rootenabled
flag has been reduced - previously you could pass a module but disable it with config. Now if you don't want a feature, just don't pass an implementation. Eg:.multiaddrs
renamed to.getMultiaddrs()
because it's not a property accessor, work is done by that method to calculate announce addresses, observed addresses, etc/p2p/${peerId}
is now appended to all addresses returned by.getMultiaddrs()
so they can be used opaquely (every consumer has to append the peer ID to the address to actually use it otherwise). If you need low-level unadulterated addresses, call methods on the address manager.BREAKING CHANGE: types are no longer hand crafted, this module is now ESM only