Skip to content
This repository has been archived by the owner on Aug 23, 2019. It is now read-only.

fix: add start method #69

Merged
merged 2 commits into from
Apr 23, 2018
Merged

Conversation

JGAntunes
Copy link
Contributor

The lack of start implies that events might get lost since we have no way of setting up handlers before ping starts emitting.

@JGAntunes
Copy link
Contributor Author

@diasdavid if you could review this when you have some please ☝️

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.

No tests? What was failing?

@JGAntunes
Copy link
Contributor Author

@diasdavid what happens is that the ping may start emitting events before handlers can be setup. Specifically - https://github.com/JGAntunes/js-ipfs/blob/feat/ping/src/core/components/ping.js#L65 - this error may not be caught since this - https://github.com/libp2p/js-libp2p-ping/blob/master/src/ping.js#L28 - might execute before the handlers are registered.
I can try to add tests to this but it's just that it looks like a kind of a complicate race condition that comes from the way the module is structured.

@JGAntunes JGAntunes mentioned this pull request Apr 6, 2018
2 tasks
@daviddias
Copy link
Member

daviddias commented Apr 23, 2018

@JGAntunes the event emitter is created before .start is called, you can always set up the handler there. Am I missing something? Anyhow, merging and releasing this one so that it can unlock the ping PR

@daviddias daviddias merged commit c0a6a46 into libp2p:master Apr 23, 2018
@ghost ghost removed the ready label Apr 23, 2018
@JGAntunes
Copy link
Contributor Author

@diasdavid sorry, I'm not following, before you had no start, which meant that the swarm.dial would be called on ping construction and stuff like this - https://github.com/libp2p/js-libp2p-ping/blob/v0.7.0/src/ping.js#L28 - for example could be emitted before you had time to setup event handlers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants