-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
Uses new CID class. Also swaps out various chai deps for version bundled with aegir, updates aegir and fixes tests that start services without tearing them down which stops the tests from finishing.
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.
Mostly minor stuff, some are learnings from @rvagg cid import flags in ipfs-repo
src/utils.js
Outdated
reject(errcode(new Error('Async function did not complete before timeout'), 'ETIMEDOUT')) | ||
}, time) | ||
|
||
if (timeout && timeout.unref) { |
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.
setTimeout
return a number, timeoutId
. Type checker is failing here as a consequence
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.
In node it returns a Timeout which has an unref
method property. Weird, maybe we need to tell ts that it's used under node.
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'm managed to refactor this away after removing the unnecessary async and using the p-timeout
module instead of Promise.race
.
@@ -28,19 +28,34 @@ export class Record implements IRecord { | |||
constructor(p?: IRecord); | |||
|
|||
/** Record key. */ | |||
public key: Uint8Array; | |||
public key?: (Uint8Array|null); |
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.
was this generated?
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, I'm not sure why this changed, new patch release of protobufjs I guess?
src/index.js
Outdated
this._running = false | ||
this.randomWalk.stop() | ||
this.providers.stop() | ||
await this.network.stop() |
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.
This does not need to be a promise, but network.stop
and network.start
need to be fixed. Registrar does not return a promise anymore: https://github.com/libp2p/js-libp2p/blob/master/src/registrar.js#L87
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've removed all of the unnecessary async.
Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>
I've marked this as a draft so we can merge it when #3556 is green as it'll bubble everything all the way up. Please do continue review and/or approve as appropriate. |
Co-authored-by: Vasco Santos <vasco.santos@moxy.studio>
…js-libp2p-kad-dht into chore/update-to-new-multiformats
Uses new CID class. Also swaps out various chai deps for version
bundled with aegir, updates aegir and fixes tests that start services
without tearing them down which stops the tests from finishing.