-
Notifications
You must be signed in to change notification settings - Fork 255
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
Implemented #172 with promises #257
base: master
Are you sure you want to change the base?
Implemented #172 with promises #257
Conversation
App.start now returns a promise which depends on all application widgets promisses. Just return a promise() in widget 'initialize' method to handle async loading.
Thanks for this change, @mbektimirov. Would it be possible to also add some docs (addition to the readme) covering this change? |
Thanks @mbektimirov ill try to review it today One comment though I went through the code quickly and my first thought is that we could probably do it in a more generic way by emitting events whenever all the widgets started via a Widget.startAll call have finished initialization... |
@sbellity +1 on using events over promises. See this good article debating their different strengths within asynchronous architecture and also my recent comment on using the Finite State Machine pattern on synchronizing events. http://www.joezimjs.com/javascript/javascript-asynchronous-architectures-events-vs-promises/ |
I agree with this. @mbektimirov just pinging you again in case you have time to update the tests. This would be hugely appreciated. |
I remember about tests, still in vacation, sorry for delay. I'll fix this in about 3-4 days. |
ping @mbektimirov in case you have time :) |
@sbellity for our next release, do you feel this is important enough to write the tests ourselves and make it more generic then land? If not, we can certainly hold off on it. |
App.start now returns a promise which depends on all application widgets promisses. Just return a promise() in widget 'initialize' method to handle async loading. Sample code is bellow.