Code review comment for lp://staging/~hatch/juju-gui/detail-hang

I'm skimming and didn't look at the service view promise support, but I
had some comments. I'm happy to talk through this tomorrow or pair if
you like.

https://codereview.appspot.com/8782046/diff/1/app/app.js
File app/app.js (right):

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode582
app/app.js:582: _prefetch_service: function(serviceId, callback,
useCallback) {
You should be able to simplify this method drastically by always using
your promise code--or maybe you can even eliminate it?

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode638
app/app.js:638: @method _servicePromise
As noted later, this is @static as written.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode654
app/app.js:654: service.set('prefetch', true);
Hm. This kind of implies a tri-state to me: not prefetched,
prefetching, and prefetched. If I called getServiceModel twice in rapid
succession, the second one would be called immediately(-ish) because
prefetch would be true the second time around. ISTM that instead you
want to look for an active "prefetching" state; and if the service is in
that state, wait to call resolve or reject until the prefetch attribute
is changed.

I'm not saying I love what I propose. I'd think about it. I am saying
that the current approach isn't quite right.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode657
app/app.js:657: db.charms.add({id: charm_id}).load(env,
Hm. Make sure that you are not duplicating code in
app/store/endpoints.js (handleServiceEvent and setupCharmOnceLoad). I
am pretty sure you are.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode661
app/app.js:661: }
You never resolve in this branch.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode677
app/app.js:677: resolve(service);
You never get the charm in this branch (except by accident, thanks to
the endpoints code).

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode689
app/app.js:689: @return {Y.Model | Y.Promise} If the service model is
readily available
The usual pattern IME is that you *always* return a promise. The
promise might already be fulfilled! In that case when you add a
callback, it will be called very, very soon. :-)

var service = this.db.services.getById(serviceId);
if (service && service.get('prefetch')) {
   return Y.when(service);
} else {
   return new Y.Promise(...);
}

Or, actually, given the way you have written _servicePromise, you could
just say

getServiceModel: function(serviceId) {
   return new Y.Promise(...);
}

This simplifies calling code, because it always deals with a promise.

https://codereview.appspot.com/8782046/diff/1/app/app.js#newcode699
app/app.js:699: Y.bind(this._servicePromise, null, serviceId, this.db,
this.env));
I'm not convinced of the value of static methods like this. I think we
should either make it a function (inline? or not?) or a method. <shrug>
I don't feel strongly about this.

https://codereview.appspot.com/8782046/