Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix: dht browser disabled #1879

Merged
merged 1 commit into from
Feb 27, 2019
Merged

fix: dht browser disabled #1879

merged 1 commit into from
Feb 27, 2019

Conversation

vasco-santos
Copy link
Member

The libp2p defaults were not allowing the browser config to disable the dht for browser nodes. This PR fixes that issue

@ghost ghost assigned vasco-santos Feb 14, 2019
@ghost ghost added the status/in-progress In progress label Feb 14, 2019
Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

Add tests :)

@vasco-santos vasco-santos force-pushed the fix/dht-browser-disabled branch from c48afe6 to c71f506 Compare February 18, 2019 16:59
@vasco-santos
Copy link
Member Author

Thanks @daviddias

I added a test running in the browser and receiving the error

@vasco-santos vasco-santos force-pushed the fix/dht-browser-disabled branch from c71f506 to 072884d Compare February 18, 2019 18:09
try {
res = await ipfs.dht.put(Buffer.from('a'), Buffer.from('b'))
} catch (err) {
expect(err).to.exist()
Copy link
Member

Choose a reason for hiding this comment

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

Is there an error code or message we can assert on?

Copy link
Member Author

Choose a reason for hiding this comment

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

No! But, I can add it to js-libp2p. However, we will have to bump the rc. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure the error is because the DHT is disabled and not because of anything else. Whatever you can do to make that happen is good 👍

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.

3 participants