Code review comment for lp:~bac/juju-gui/get-endpoints

Revision history for this message
Benjamin Saller (bcsaller) wrote :

This is looking good but I do ask a structural change below. I hope I
explain it well enough (and the reasons for it). If you have questions
let me know.

https://codereview.appspot.com/8275043/diff/2001/app/app.js
File app/app.js (left):

https://codereview.appspot.com/8275043/diff/2001/app/app.js#oldcode478
app/app.js:478:
Nice that this removed so much code from App :)

https://codereview.appspot.com/8275043/diff/2001/app/app.js
File app/app.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/app.js#newcode364
app/app.js:364: this);
This is fine for now but When I have this many subscriptions I'd usually
wrap it in a controller object such that

controller._subscriptions = controller.bind()

and controller.unbind can use the subscription object to remove
listeners.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js
File app/models/endpoints.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode29
app/models/endpoints.js:29: models.getEndpoints = function(svc, ep_map,
db) {
This signature doesn't work well anymore, the db may or may not be the
one the service handlers were bound to. By making a controller object we
can reference the db when we bind the handlers and drop this argument
making it consistent.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode167
app/models/endpoints.js:167: charm.on('load', Y.bind(function(svcName,
evt) {
It shouldn't matter, but this seems like a case for charm.once(...)
rather than on() as the charm should be static post load and we don't
need to keep the listener active.

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode181
app/models/endpoints.js:181: models.serviceAddHandler = function(evt) {
If we had a controller here like EndpointManager or something we could
hang the handlers off that rather than models which would be nicer (In
addition to the bind/unbind stuff mentioned before).

https://codereview.appspot.com/8275043/diff/2001/app/models/endpoints.js#newcode254
app/models/endpoints.js:254: for (var k in meta) {
Why is this done in native JS rather than with each() or map()? (I
think) The linter complaining here is because in a loop we want to know
if the current item is owned by the object or part of its prototype, the
higher level constructs can safely handle those cases for us.

https://codereview.appspot.com/8275043/diff/2001/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (left):

https://codereview.appspot.com/8275043/diff/2001/app/store/env/fakebackend.js#oldcode427
app/store/env/fakebackend.js:427: // },
nice catch, thanks for pulling all these from the envs

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation.js
File app/views/topology/relation.js (right):

https://codereview.appspot.com/8275043/diff/2001/app/views/topology/relation.js#newcode563
app/views/topology/relation.js:563: var endpoints =
models.getEndpoints(service, models.endpoints_map, db);
as noted before, in causes like this we can't always be sure the db is
the one the listeners were bound to.

https://codereview.appspot.com/8275043/

« Back to merge proposal