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

fix: init event #1293

Closed
wants to merge 2 commits into from
Closed

fix: init event #1293

wants to merge 2 commits into from

Conversation

wraithgar
Copy link
Contributor

For #1058

The init event was only being emitted during initialization of
a new repo. If one already existed that event was not firing.
This syncs up the behavior between these two situations so that they
log, set state, and emit identically.

The init event was only being emitted during initialization of
a new repo.  If one already existed that event was not firing.
This syncs up the behavior between these two situations so that they
log, set self.state, and emit identically.
@ghost ghost assigned wraithgar Apr 2, 2018
@ghost ghost added the status/in-progress In progress label Apr 2, 2018
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.

Thanks @wraithgar. Mind adding a test to make sure we don't break this in the future?

src/core/boot.js Outdated
@@ -32,6 +32,7 @@ module.exports = (self) => {
(cb) => {
self.log('initialized')
self.state.initialized()
self.emit('init');
Copy link
Member

Choose a reason for hiding this comment

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

This will failing linting (semicolons)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. Linting already reports 87 problems for me in master so that one got lost in the noise. Pushed a fix.

@wraithgar
Copy link
Contributor Author

@diasdavid I'd be glad to add tests but would need a little help to be pointed in the right direction. As far as I can see in this repo there isn't anything testing the events firing. Am I just not looking in the right direction or would these be wholly new?

@daviddias
Copy link
Member

@Mr0grog
Copy link
Contributor

Mr0grog commented Apr 3, 2018

Isn’t the point of the init event to indicate that the repo was init’d? This completely changes the semantics of that event and removes a useful signal (you can’t tell from the outside whether init will happen; the boot process chooses to do so based on what it finds at the repo location). Also, it will emit twice here in the case where the repo does get init’d. That seems bad.

I feel like this event should be named something different that indicates what it is actually about. Maybe prestart or prepared or repo-opened or something?

@wraithgar
Copy link
Contributor Author

wraithgar commented Apr 3, 2018

It won't fire twice here.

In boot.js, self.init is only called if maybeOpenRepo returns false, and the callback after the emitted init event returns true to the callback.

As far as the semantics of the init event, this is what I thought being discussed in #1058. I may have misunderstood. It would be possible to just emit a peer or repoevent once the peer is set instead in the pre-start component.

@Mr0grog
Copy link
Contributor

Mr0grog commented Apr 3, 2018

It won't fire twice here.

Ah, you’re right; I misread.

As far as the semantics of the init event, this is what I thought being discussed in #1058.

I don’t think the semantics of the event really got any discussion there, though. Nobody actually said what the event was doing was wrong or unexpected — only that it didn’t fit @pgte’s use case.

I think there are two different things in that issue that make more sense to address:

  • Is ready always emitting when constructing an IPFS node (assuming it didn’t have any errors)? If not, that’s probably a bug that needs fixing.

  • @pgte’s specific ask was “I would like to have an event that tells me once the peerId is known.” Does the above with ready solve that? If not, what’s the right event to add? It’s probably something right where you added init here (or maybe inside preStart), I just think it needs a different name because it’s a different thing :)

@wraithgar
Copy link
Contributor Author

If it's a new event it makes sense to do it right in pre-start after self._peerInfo is defined (because from that moment on calls to self.id() will return something).

@wraithgar
Copy link
Contributor Author

The matter of ready always firing also does not meet the use case, from what I can tell. I think he simply wanted to know the moment that a peer was known so that operations that required one could start, without having to wait on the swarm et al operations to consider themselves ready.

@Mr0grog
Copy link
Contributor

Mr0grog commented Apr 3, 2018

Fair enough! But maybe useful to wait for feedback from @pgte and some discussion on the original issue to zero in on the right change before continuing with the implementation here?

@alanshaw
Copy link
Member

@wraithgar can this be closed? Reading the comments above it sounds as though the init event is being fired correctly already?

@wraithgar wraithgar closed this May 29, 2018
@ghost ghost removed the status/in-progress In progress label May 29, 2018
@wraithgar wraithgar deleted the init_event branch May 29, 2018 15:54
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.

4 participants