fix: add maxtimeout to dht get#248
Conversation
ghost
assigned
jacobheun
left a comment
jacobheun
left a comment
There was a problem hiding this comment.
Since the functions are changing in this PR we should make sure it doesn't break the current usage (even if the current usage is wrong). Let's check if maxTimeout is actually the callback and handle that accordingly.
We should also add basic mock dht tests to libp2p. We don't need to validate the dht, but we should be validating that when the dht is enabled and put,get,getMany are called with and without timeouts, that the mocks are properly called.
We should also verify we get errors when the dht is not enabled.
vasco-santos
force-pushed
the
fix/add-maxtimeout-to-dht-get
branch
from
d076f49 to
d3dd11f
Compare
|
hey @jacobheun I updated the PR with your feedback 🙂 |
|
Woot, looks good! |
| node._dht.put(key, value, callback) | ||
| }, | ||
| get: (key, callback) => { | ||
| get: (key, maxTimeout, callback) => { |
There was a problem hiding this comment.
things like timeouts should be in a options object rather than a arg otherwise you will fall into a future of having a huge array of options. @vasco-santos @jacobheun
There was a problem hiding this comment.
You are right! I will create a new PR for changing it here, as well as for js-libp2p-kad-dht and will change js-ipfs too.
3 participants
In the context of having the
DHTenabled by default injs-ipfs, several problems were identified on js-ipfs/856#issuecomment-422495198. In this case,js-libp2pdoes not allow the definition of amaxTimeout.maxTimeoutand thecallbackis lost.NOTE: There are no tests for the DHT in
js-ipfs, is there any reason?