Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

fix: dht.findProvs.js handle valid hash but no providers #950

Conversation

niinpatel
Copy link
Contributor

@niinpatel niinpatel commented Mar 2, 2019

Resolves #928

@niinpatel niinpatel force-pushed the fix/findProvs-valid-hash-but-no-providers branch from 646ae57 to 3fc1641 Compare March 2, 2019 04:44
@niinpatel niinpatel changed the title fix: dht.findProvs.js handle valid hash but no fix: dht.findProvs.js handle valid hash but no providers Mar 2, 2019
@niinpatel niinpatel marked this pull request as ready for review March 2, 2019 04:52
@niinpatel niinpatel force-pushed the fix/findProvs-valid-hash-but-no-providers branch 2 times, most recently from 486e1d9 to 52cd436 Compare March 2, 2019 07:01
@niinpatel niinpatel force-pushed the fix/findProvs-valid-hash-but-no-providers branch from 52cd436 to 49b16ec Compare March 10, 2019 04:03
alanshaw
alanshaw previously approved these changes Mar 14, 2019
@alanshaw alanshaw dismissed their stale review March 14, 2019 14:18

Problems noticed after initial review

@@ -23,6 +23,12 @@ module.exports = (send) => {
}

const handleResult = (res, callback) => {
// callback with an empty array if no providers are found
if (res === null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the issue, res is undefined so this change won't resolve the issue. Suggestion below (which will catch both null and undefined):

Suggested change
if (res === null) {
if (res == null) {

A small unit test to assert this, that mocks out send would have caught this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or instead of double equals, we can do if (!res) { ... }

@niinpatel niinpatel force-pushed the fix/findProvs-valid-hash-but-no-providers branch from 49b16ec to 71f5c21 Compare March 14, 2019 16:22
@niinpatel niinpatel force-pushed the fix/findProvs-valid-hash-but-no-providers branch from 71f5c21 to 75552f6 Compare March 14, 2019 16:25
@alanshaw alanshaw merged commit c3cde76 into ipfs-inactive:master Mar 15, 2019
@niinpatel niinpatel deleted the fix/findProvs-valid-hash-but-no-providers branch March 15, 2019 21:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants