Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

started => booted [WAITING FOR npm PUBLISH OF loopback-boot] #267

Closed
wants to merge 1 commit into from

Conversation

AshKyd
Copy link

@AshKyd AshKyd commented Feb 3, 2017

The changelog indicates the booted event was added in 2015-01-08, Version 2.6.0

For reference, the emit is called here https://github.com/strongloop/loopback-boot/blob/master/lib/plugins/application.js#L131

@slnode
Copy link
Contributor

slnode commented Feb 3, 2017

Can one of the admins verify this patch?

The changelog indicates the `booted` event was added in
2015-01-08, Version 2.6.0

For reference, the emit is called here:
https://github.com/strongloop/loopback-boot/blob/master/lib/plugins/app
lication.js#L131
@crandmck
Copy link
Contributor

crandmck commented Feb 3, 2017

This change looks correct to me. @superkhau can you pls confirm...?
Bummed that the docs have been wrong for so long!

@superkhau superkhau self-requested a review February 3, 2017 22:44
@superkhau
Copy link
Contributor

@crandmck I believe this is correct. @bajtos Can you confirm? I can't recall if it has both a started and a booted event -- simple grep of lb + lb-boot doesn't show emit('started'.

@bajtos
Copy link
Member

bajtos commented Feb 6, 2017

The source line points to code that's part of the upcoming 3.0 release of loopback-boot, which didn't happen yet (cc @raymondfeng @davidcheung). As far as I understand the new loopback-boot code, it's true the app will emit booted when loopback-boot finishes its work.

However, the started event will be emitted too, as this is part of loopback scaffolding - see app.start() in server/server.js template.

I am proposing to keep the current text without changes and add a new paragraph mentioning the new booted event produced by the upcoming loopback-boot version.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

☝️

@crandmck
Copy link
Contributor

crandmck commented Feb 6, 2017

Oops, I accidentally approved this, but IIUC, according to @bajtos we should leave the existing text "as is" and then add the booted event when loopback-boot v3.0 is published.

I don't think adding this information before it's published to npm makes sense. So if that's going to happen soon, we can leave this PR open (with the suggested changes). If not, then we should close this and maybe just open a tracking issue to add that when it does occur.

@crandmck
Copy link
Contributor

crandmck commented Mar 8, 2017

@AshKyd Do you want to make the changes suggested above ?
If not, I'll close this....

@bajtos
Copy link
Member

bajtos commented Mar 9, 2017

I am afraid loopback-boot@3 was still not released yet - see strongloop/loopback-boot#241

@bajtos bajtos mentioned this pull request Mar 9, 2017
3 tasks
@crandmck
Copy link
Contributor

Yes, it still has label waiting. But I thought we could go ahead and do this...

keep the current text without changes and add a new paragraph mentioning the new booted event produced by the upcoming loopback-boot version.

Unless I'm misunderstanding...?

@AshKyd
Copy link
Author

AshKyd commented Mar 10, 2017

No probs, will make the change shortly.

@AshKyd
Copy link
Author

AshKyd commented Mar 15, 2017

After trying to write it, I don't think I that suggestion really works. Would something like the following be better?

By default, an application emits the 'booted' event when it starts up, after running boot scripts. In addition, an application created with the application generator emits a 'started' event.

I'm not 100% sure this is right, but the reason I originally opened this ticket was because my app wasn't emitting started. I think this version text it clearer where that event comes from. Can someone please double-check before I make the changes?

@crandmck crandmck changed the title started => booted started => booted [WAITING FOR npm PUBLISH OF loopback-boot] Apr 12, 2017
@crandmck
Copy link
Contributor

@bajtos It looks like loopback-boot was published to npm some time ago. Should we land this now?

@bajtos
Copy link
Member

bajtos commented May 22, 2017

@crandmck

It looks like loopback-boot was published to npm some time ago. Should we land this now?

In my understanding, we are waiting specifically for the version 3.0.0 to be published. AFAICT, that did not happen yet, see https://github.com/strongloop/loopback-boot/releases and https://www.npmjs.com/package/loopback-boot.

We have an issue to track the task of releasing [email protected], see strongloop/loopback-boot#241

@AshKyd
Copy link
Author

AshKyd commented May 15, 2019

Closing this because it's likely no longer relevant and I don't want it in my list.

@AshKyd AshKyd closed this May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants