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

fix: better error message when pubsub is not enabled #1729

Merged
merged 1 commit into from Nov 27, 2018

Conversation

alanshaw
Copy link
Member

If pubsub is disabled there's no pubsub property on the libp2p node and we get the error "Cannot read property 'subscribe' of undefined". This is really confusing. This PR detects disabled pubsub and throws an error with a better message.

If pubsub is disabled there's no `pubsub` property on the libp2p node and we get the error "Cannot read property 'subscribe' of undefined". This is really confusing. This PR detects disabled pubsub and throws an error with a better message.

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@ghost ghost assigned alanshaw Nov 26, 2018
@ghost ghost added the status/in-progress In progress label Nov 26, 2018
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

I have been thinking that we should have a getter for pubsub and dht exposed from libp2p, and the error in that case would come from there.

In my opinion, we can move with this solution and if you and @jacobheun agree, I can implement the getters with the errors in js-libp2p and update js-ipfs later.

Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

I'd like to take advantage of the await/async migration and revisit libp2p's api to help avoid applications needing to do these kind of checks, it's annoying to need to check every method like this.

I think the libp2p public apis should always be invokable and error appropriately. libp2p core is really just exposing services that have been bundled with it. We should ensure those api calls are simple and informative when they do error. I'd expect to be able to just call libp2p.pubsub.ls, etc, and have it return this type of error.

This is fine as a interim solution, but libp2p should work to make sure this commit is revertible in the near future.

I'll get with @vasco-santos and we can talk through some options for improving the API and create a proposal for feedback for the migration.

@alanshaw
Copy link
Member Author

alanshaw commented Nov 27, 2018

I think what @vasco-santos suggested is an interesting idea, but it would need to wait for the async/await refactor, otherwise we'd still need code to deal with the throw in js-ipfs and return it in the callback.

libp2p.pubsub can be a getter that throws if pubsub is not enabled. When we have an async/await API, an app using libp2p will (likely) have a try/catch around the call anyway:

class Lib2p {
  /* ... */
  get pubsub () {
    if (!this.options.pubsub.enabled) throw new Error('pubsub not enabled')
    return this._pubsub
  }
}
const libp2p = new Libp2p

try {
  await libp2p.pubsub.subscribe(/*...*/)
} catch (err) {
  console.log(err.message) // pubsub is not enabled
}

@alanshaw alanshaw merged commit 5237dd9 into master Nov 27, 2018
@alanshaw alanshaw deleted the fix/better-err-for-pubsub-disabled branch November 27, 2018 11:08
@ghost ghost removed the status/in-progress In progress label Nov 27, 2018
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.

None yet

3 participants