-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
UI: Service Instances #5326
UI: Service Instances #5326
Changes from all commits
7610111
aed28a7
9e73e84
1e7cab0
bc649a5
60f8b14
c65ec2d
2959141
0702b4e
f7d4637
6bb9ebb
bd508c0
9719d01
bde111b
5163a2b
daa19ca
39982e8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import Adapter from './application'; | ||
import { PRIMARY_KEY, SLUG_KEY } from 'consul-ui/models/proxy'; | ||
import { OK as HTTP_OK } from 'consul-ui/utils/http/status'; | ||
export default Adapter.extend({ | ||
urlForQuery: function(query, modelName) { | ||
if (typeof query.id === 'undefined') { | ||
throw new Error('You must specify an id'); | ||
} | ||
// https://www.consul.io/api/catalog.html#list-nodes-for-connect-capable-service | ||
return this.appendURL('catalog/connect', [query.id], this.cleanQuery(query)); | ||
}, | ||
handleResponse: function(status, headers, payload, requestData) { | ||
let response = payload; | ||
if (status === HTTP_OK) { | ||
const url = this.parseURL(requestData.url); | ||
response = this.handleBatchResponse(url, response, PRIMARY_KEY, SLUG_KEY); | ||
} | ||
return this._super(status, headers, response, requestData); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
import Component from '@ember/component'; | ||
export default Component.extend({ | ||
tagName: '', | ||
}); | ||
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||
import Component from '@ember/component'; | ||||
import { get } from '@ember/object'; | ||||
|
||||
export default Component.extend({ | ||||
// TODO: Could potentially do this on attr change | ||||
actions: { | ||||
sortChecksByImportance: function(a, b) { | ||||
const statusA = get(a, 'Status'); | ||||
const statusB = get(b, 'Status'); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is where Enums are nice. This sort is essentially ordering everything Critical, then Warning, then Passing? You could do... const precedence = {
passing: 0,
warning: 1,
critical: 2,
};
// ...
sortChecksByImportance: function(a, b) {
const statusA = precedence[get(a, 'Status')];
const statusB = precedence[get(b, 'Status')];
return statusB - statusA;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah nice! Very helpful, thankyou. When I come to refactor this stuff I can use this. |
||||
switch (statusA) { | ||||
case 'passing': | ||||
// a = passing | ||||
// unless b is also passing then a is less important | ||||
return statusB === 'passing' ? 0 : 1; | ||||
case 'critical': | ||||
// a = critical | ||||
// unless b is also critical then a is more important | ||||
return statusB === 'critical' ? 0 : -1; | ||||
case 'warning': | ||||
// a = warning | ||||
switch (statusB) { | ||||
// b is passing so a is more important | ||||
case 'passing': | ||||
return -1; | ||||
// b is critical so a is less important | ||||
case 'critical': | ||||
return 1; | ||||
// a and b are both warning, therefore equal | ||||
default: | ||||
return 0; | ||||
} | ||||
} | ||||
return 0; | ||||
}, | ||||
}, | ||||
}); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This component is now used to render healthcheck 'outputs' i.e. the log information and further detail for the healthchecks, it basically groups
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import Component from '@ember/component'; | ||
|
||
export default Component.extend({ | ||
classNames: ['healthcheck-output'], | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,12 @@ | ||
import Component from '@ember/component'; | ||
|
||
import { get, computed } from '@ember/object'; | ||
export default Component.extend({ | ||
classNames: ['healthcheck-status'], | ||
tagName: '', | ||
count: computed('value', function() { | ||
const value = get(this, 'value'); | ||
if (Array.isArray(value)) { | ||
return value.length; | ||
} | ||
return value; | ||
}), | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import Component from '@ember/component'; | ||
|
||
export default Component.extend({ | ||
tagName: 'dl', | ||
classNames: ['tag-list'], | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tag listing is now being used in various places, so it made sense to make it into a simple component to centralize the markup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. big plus one to this, I didn't do this early enough in vault and we now need to make a pass to contain markup for various things and bundle them into components. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah there's more to come here. There's a lot in the feature that requires me to reuse things that originally were only in one place but are now in multiple places. I quibbled a lot about what to change in this PR as I didn't want to move too much around. Following on from this there is likely to be a lot of 'lets make that a component now its used in more than one place'. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import Controller from '@ember/controller'; | ||
import { set } from '@ember/object'; | ||
|
||
export default Controller.extend({ | ||
setProperties: function() { | ||
this._super(...arguments); | ||
// This method is called immediately after `Route::setupController`, and done here rather than there | ||
// as this is a variable used purely for view level things, if the view was different we might not | ||
// need this variable | ||
set(this, 'selectedTab', 'service-checks'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to encode selectedTab as a query param? Or a child route? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought the same originally, it did use to be like this but it was decided not to 'remember' it. (Remember this is all implemented in the same way as the rest of the app, I just copied the plumbing over to here) |
||
}, | ||
actions: { | ||
change: function(e) { | ||
set(this, 'selectedTab', e.target.value); | ||
}, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,38 +1,39 @@ | ||
import Controller from '@ember/controller'; | ||
import { get, computed } from '@ember/object'; | ||
import sumOfUnhealthy from 'consul-ui/utils/sumOfUnhealthy'; | ||
import hasStatus from 'consul-ui/utils/hasStatus'; | ||
import WithHealthFiltering from 'consul-ui/mixins/with-health-filtering'; | ||
import { get, set, computed } from '@ember/object'; | ||
import { inject as service } from '@ember/service'; | ||
import WithSearching from 'consul-ui/mixins/with-searching'; | ||
export default Controller.extend(WithSearching, WithHealthFiltering, { | ||
export default Controller.extend(WithSearching, { | ||
dom: service('dom'), | ||
init: function() { | ||
this.searchParams = { | ||
healthyServiceNode: 's', | ||
unhealthyServiceNode: 's', | ||
serviceInstance: 's', | ||
}; | ||
this._super(...arguments); | ||
}, | ||
searchableHealthy: computed('healthy', function() { | ||
return get(this, 'searchables.healthyServiceNode') | ||
.add(get(this, 'healthy')) | ||
.search(get(this, this.searchParams.healthyServiceNode)); | ||
}), | ||
searchableUnhealthy: computed('unhealthy', function() { | ||
return get(this, 'searchables.unhealthyServiceNode') | ||
.add(get(this, 'unhealthy')) | ||
.search(get(this, this.searchParams.unhealthyServiceNode)); | ||
}), | ||
unhealthy: computed('filtered', function() { | ||
return get(this, 'filtered').filter(function(item) { | ||
return sumOfUnhealthy(item.Checks) > 0; | ||
}); | ||
}), | ||
healthy: computed('filtered', function() { | ||
return get(this, 'filtered').filter(function(item) { | ||
return sumOfUnhealthy(item.Checks) === 0; | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We no longer filter by healthcheck status in the 'Service Instances' page |
||
setProperties: function() { | ||
this._super(...arguments); | ||
// This method is called immediately after `Route::setupController`, and done here rather than there | ||
// as this is a variable used purely for view level things, if the view was different we might not | ||
// need this variable | ||
set(this, 'selectedTab', 'instances'); | ||
}, | ||
searchable: computed('items', function() { | ||
return get(this, 'searchables.serviceInstance') | ||
.add(get(this, 'items')) | ||
.search(get(this, this.searchParams.serviceInstance)); | ||
}), | ||
filter: function(item, { s = '', status = '' }) { | ||
return hasStatus(get(item, 'Checks'), status); | ||
actions: { | ||
change: function(e) { | ||
set(this, 'selectedTab', e.target.value); | ||
// Ensure tabular-collections sizing is recalculated | ||
// now it is visible in the DOM | ||
get(this, 'dom') | ||
.components('.tab-section input[type="radio"]:checked + div table') | ||
.forEach(function(item) { | ||
if (typeof item.didAppear === 'function') { | ||
item.didAppear(); | ||
} | ||
}); | ||
}, | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import Model from 'ember-data/model'; | ||
import attr from 'ember-data/attr'; | ||
|
||
export const PRIMARY_KEY = 'uid'; | ||
export const SLUG_KEY = 'ID'; | ||
export default Model.extend({ | ||
[PRIMARY_KEY]: attr('string'), | ||
[SLUG_KEY]: attr('string'), | ||
ServiceName: attr('string'), | ||
ServiceID: attr('string'), | ||
ServiceProxyDestination: attr('string'), | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We introduce a new 'noun' or model here - a Proxy, a 'special' type of Service Instance |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,9 @@ export const routes = { | |
show: { | ||
_options: { path: '/:name' }, | ||
}, | ||
instance: { | ||
_options: { path: '/:name/:id' }, | ||
}, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our routing uses a declarative format now. See here for information: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was declarative before too 😛 (but good reminder!) |
||
// Nodes represent a consul node | ||
nodes: { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import Route from '@ember/routing/route'; | ||
import { inject as service } from '@ember/service'; | ||
import { hash } from 'rsvp'; | ||
import { get } from '@ember/object'; | ||
|
||
export default Route.extend({ | ||
repo: service('repository/service'), | ||
proxyRepo: service('repository/proxy'), | ||
model: function(params) { | ||
const repo = get(this, 'repo'); | ||
const proxyRepo = get(this, 'proxyRepo'); | ||
const dc = this.modelFor('dc').dc.Name; | ||
return hash({ | ||
item: repo.findInstanceBySlug(params.id, params.name, dc), | ||
}).then(function(model) { | ||
return hash({ | ||
proxy: | ||
get(service, 'Kind') !== 'connect-proxy' | ||
? proxyRepo.findInstanceBySlug(params.id, params.name, dc) | ||
: null, | ||
...model, | ||
}); | ||
}); | ||
}, | ||
setupController: function(controller, model) { | ||
this._super(...arguments); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if you still want to call super here - pretty sure just does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @meirish Sorry, I forgot about this and merged! Agree that it's questionable whether I'm going to look into this a little further. I'm not sure if it only sets variables on the controller for you. I vaguely remember reading a blog article saying you should always call Up until now I've found it really difficult to know whether something in ember is deemed a 'hook' (and therefore a super call isn't needed) and when something is 'accepted overriding of a method' and therefore a Anyway lemme look into it further. Thanks There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok found the blog post: But, reading that more carefully and looking at the ember source: The 'things can go wrong' is that a model variable isn't set in the template context/controller. In my case I never use Thanks, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a PR here #5383 |
||
controller.setProperties(model); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ export default Route.extend({ | |
return { | ||
...model, | ||
...{ | ||
// Nodes happen to be the ServiceInstances here | ||
items: model.item.Nodes, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A note here to the old 'noun' There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are they still There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah the API is still the same as always. The API here talks about |
||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import Serializer from './application'; | ||
import { PRIMARY_KEY } from 'consul-ui/models/proxy'; | ||
|
||
export default Serializer.extend({ | ||
primaryKey: PRIMARY_KEY, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import RepositoryService from 'consul-ui/services/repository'; | ||
import { PRIMARY_KEY } from 'consul-ui/models/proxy'; | ||
import { get } from '@ember/object'; | ||
const modelName = 'proxy'; | ||
export default RepositoryService.extend({ | ||
getModelName: function() { | ||
return modelName; | ||
}, | ||
getPrimaryKey: function() { | ||
return PRIMARY_KEY; | ||
}, | ||
findAllBySlug: function(slug, dc, configuration = {}) { | ||
const query = { | ||
id: slug, | ||
dc: dc, | ||
}; | ||
if (typeof configuration.cursor !== 'undefined') { | ||
query.index = configuration.cursor; | ||
} | ||
return this.get('store').query(this.getModelName(), query); | ||
}, | ||
findInstanceBySlug: function(id, slug, dc, configuration) { | ||
return this.findAllBySlug(slug, dc, configuration).then(function(items) { | ||
if (get(items, 'length') > 0) { | ||
const instance = items.findBy('ServiceProxyDestination', id); | ||
if (instance) { | ||
return instance; | ||
} | ||
} | ||
return; | ||
}); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,16 +7,35 @@ export default RepositoryService.extend({ | |
}, | ||
findBySlug: function(slug, dc) { | ||
return this._super(...arguments).then(function(item) { | ||
const nodes = get(item, 'Nodes'); | ||
const service = get(nodes, 'firstObject'); | ||
const tags = nodes | ||
.reduce(function(prev, item) { | ||
return prev.concat(get(item, 'Service.Tags') || []); | ||
}, []) | ||
.uniq(); | ||
set(service, 'Tags', tags); | ||
set(service, 'Nodes', nodes); | ||
return service; | ||
const nodes = get(item, 'Nodes'); | ||
const service = get(nodes, 'firstObject'); | ||
const tags = nodes | ||
.reduce(function(prev, item) { | ||
return prev.concat(get(item, 'Service.Tags') || []); | ||
}, []) | ||
.uniq(); | ||
set(service, 'Tags', tags); | ||
set(service, 'Nodes', nodes); | ||
return service; | ||
}); | ||
}, | ||
findInstanceBySlug: function(id, slug, dc, configuration) { | ||
return this.findBySlug(slug, dc, configuration).then(function(item) { | ||
const i = item.Nodes.findIndex(function(item) { | ||
return item.Service.ID === id; | ||
}); | ||
if (i !== -1) { | ||
const service = item.Nodes[i].Service; | ||
service.Node = item.Nodes[i].Node; | ||
service.ServiceChecks = item.Nodes[i].Checks.filter(function(item) { | ||
return item.ServiceID != ''; | ||
}); | ||
service.NodeChecks = item.Nodes[i].Checks.filter(function(item) { | ||
return item.ServiceID == ''; | ||
}); | ||
return service; | ||
} | ||
// TODO: probably need to throw a 404 here? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll probably look to add 404 checking after this goes in. It's probably unlikely here, and as such will definitely happen at some point 😁 We plan on adding a lot more testing ontop of this, so this will probably happen in tandem with that. |
||
}); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,15 @@ | |
display: flex; | ||
align-items: flex-start; | ||
} | ||
%app-view header dl { | ||
float: left; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Floats instead of flexbox? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally I'll reach for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should, and you can get finicky with things like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @DingoEatingFuzz , The property to use to implement this with Before I go further into this. I'd be interested to know what the basis of:
..is? I have my opinions on using a Thanks, John There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name says it all: clearfix. It's a workaround caused by float never being meant for layouts. Sure, it works like a charm, but
There isn't one property to use. It's going to take a combination of things.
It isn't a critical issue, so there is not need to go further into it unless you have time on your hands. I only bring it up because I see flexbox as the successor to this float/clearfix pattern. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Your exact reasons for not using something is just because of its name? My exact reasons for not using an extra clearfix div is because I try to avoid adding markup to templates just for styling concerns, it makes it harder to build a responsive webapp. It's important to note here, I don't use a clearfix anywhere here. This is another Straw man Completely agree it's not critical and this isn't important in the scheme of things, and I definitely don't have time on my hands! But it is related to something that keeps coming up. I do use |
||
margin-top: 25px; | ||
margin-right: 50px; | ||
margin-bottom: 20px; | ||
} | ||
%app-view header dt { | ||
font-weight: bold; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
/* units */ | ||
%app-view { | ||
margin-top: 50px; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've used our pure computed property helper here (details here #5079). I did wonder whether to start using this here or not as I'm unsure of what impact this has when/if we eventually move to decorators/
@computed
. My only concern here is having to create our own@subscribe
decorator and having to switch out all instances of this to use our own decorator. We figured in the scheme of things it won't be much effort to do that manually.An interesting point here is the benefit of the pure function, it's concerned with only the arguments that you pass to it and returning a value. It doesn't matter what the name of the property is for example:
..using a pure computed function and thinking about
subscribe
ing to when a property changes, means this code easily becomes less verbose. Last little note here I'm not 100% convinced on thesubscribe
word here, but I definitely didn't want to reusecomputed
for clarity.We'd be happy for this to use traditional
computed
if anyone had concerns here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would typically write this has a computed property macro:
Which eliminates the concern of
computed
vs.subscribed
.I'm not sure how I feel about the name subscribes, since I would expect to see computed, but I'm also not a fan of calling something computed that isn't
Ember.computed
, sosubscribe
seems like a fine name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to naming, how do you feel about partial application for the
purify
helper? Something like...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't these (at least in this case) use the built-in computed macro
alias
?passingCount: alias('passing.length')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alias won't account for the case where
passing
isn't a length but is instead the count value already.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, thanks - thought there might be a regular input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DingoEatingFuzz , I would like to replace all
computed
withsubscribe
throughout the app. I prefer the pure approach, pass arguments in - return something out, touch as little as possible apart from what is passed in via arguments:If you get time take another read of the PR for the
purify
thing, there's a lot of info there on other reasons why I prefer it.Anyway time for me to finish up, lets chat Monday, have a good weekend!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have read the purify PR and I honestly don't get the motivation. I see the difference, and I agree with both yours and Tom's reasoning that dependent keys are nothing more than "hints" for Ember, but if this is a problem that will be addressed in Ember core, why take it on as your own problem?
Now what you have is an incomplete leaky abstraction over the top of
Ember.computed
that has no documentation.Incomplete because as you mention, the
args.map
tries its hardest to convert dependent keys into the correct object properties, but it doesn't support the entire DSL (not to mention the DSL has ambiguity since it isn't necessarily developer intent).Leaky because as a developer you still have to be conscious of
Ember.computed
, how it works and the dependent key DSL.It just doesn't seem pragmatic to attempt to gloss over a cornerstone feature of the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @DingoEatingFuzz
Sorry, I just meant to give it another pass if you had time. The main reason for
purify
is to make the function you pass tocomputed
pure (which is why it's calledpurify
). From your comments above I didn't get the impression that you'd taken that away so thought it might be useful to take another pass at it. For example:If the main reason/concern for
purify
is to make the function you pass tocomputed
pure, then let's use a good definition of a pure function to begin:In your code above:
This/
this
isn't an input value. To address your point directly, the concern ofcomputed
vssubscribe
isn't eliminated here.Maybe a familiar example might help:
https://github.com/hashicorp/nomad/blob/f29c3e81a2ab69474c105a85cd8ef4d87830b8f8/ui/app/controllers/jobs/index.js#L135-L177
could be rewritten as:
My reasons for doing this are that it's much easier to read and follow what's happening, and you get the additional benefit of being able to use default arguments. Good for type hinting and good for avoiding more complicated defensive coding. I can also hoist the function up and out if I want without having to worry about depending on
this
orget
. I can then reuse it and/or test it in isolation.Considering we use
computed
a lot in our codebases, which version of the above would you prefer to write multiple times?I've read over your other points here a few times and wondered how to respond. I think it's important and fair that I touch on your points here, but I don't want to get too far into the weeds. More than happy to chat about this later if you need more clarification.
Overall I think we can all agree with Tom that this is a problematic. I don't see
purify
as a 'solution to a problem' more of a slight improvement.I've decided to write these ~10-20 lines of code to help me reduce boilerplate, which in turn helps our users. As and when the ember core team release a different approach, I'll consider using that instead:
Your other points:
In my eyes these are Straw Man arguments.
it could potentially be made to do so, but we don't need that right now at least
We don't need it so we didn't add it. This is not something that is meant to be shared, we are the only authors and consumers. It is 'complete' for what we want to use it for. Have we finished working on this? Maybe, maybe not. The only thing it doesn't support is Ember's unique syntax. If we want to use that we could just use
alias
first.As a comparison, as far as I'm aware Ember angle bracket syntax isn't supported during testing, and doesn't support all types of components for example
{{link-to}}
. IMO this is more 'incomplete' - it doesn't stop anyone from using it though.So, it's not incomplete, I could add extra features that I don't need, yes, but not 'incomplete'.
This one really confused me.
...because as a developer I have to be conscious of
Ember.computed
? I don't understand completely how this phrase defines the term 'leaky'? Leaky as in abstraction leaky? As in leaking the details of the underlying implementation?passingCount subscribes to changes to passing and counts the value
I don't see how the underlying implementation is 'leaked' anywhere here. Am I missing something? If I change the name of
count
tolengthOrValue
instead, then yes that would be leaking the implementation of counting into the abstraction (in this case, the name of the function).I am aware that it happens to use
computed
just because I wrote it, but I guess I could implement what I've done using something else, like traditional EventListeners or maybe ember observables. I wouldn't have to change how I would use this if I did so.There is no leaky abstraction here. There is no mention of
computed
,get
or actually anything to do with Ember. It could be usingcomputed
, or observables, or maybe evenEventSource
s - and you'd never know.Maybe I've misunderstood what you meant.
I'm not sure what you meant by 'tries its hardest'? Sounds terrible! One of the aims of a Straw Man argument is to cause alarm. The code here doesn't 'try its hardest', it doesn't try at all, hopefully the comment in the code explains that.
Most of the Consul UI is undocumented, the same for Vault and Nomad. For example, I've also noticed you do something very similar here without any documentation:
https://github.com/hashicorp/nomad/blob/f29c3e81a2ab69474c105a85cd8ef4d87830b8f8/ui/app/controllers/clients/index.js#L40-L43
https://github.com/hashicorp/nomad/blob/f29c3e81a2ab69474c105a85cd8ef4d87830b8f8/ui/app/utils/qp-serialize.js
To be fair, here you are factually correct that there is no real documentation for this function. I'd be happy to add some if you think its useful. But I think that in the scheme of things this just isn't really important, which is why I'd still see this as "attacking a Straw Man"
Lastly, there is one and only one reason why I am hesitant to use
purify
and that is because at some point we'll be able to write code like:My concern is the upgrade path from
subscribe
to be able to do this. I've considered this and decided that I am still ok usingsubscribe
. Why?:subscribe
say by wrapping it up like you have done above, instead I compose functionality. I can drop back easily if need be:@subscribe
decorator, like other people have written their own decorators.Hopefully that helps to explain where I'm coming from. Feel free to ping me offline if you'd like to discuss further.
Thanks
John
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping over your explanation of what purify is, since I already told you I know what it is, what it does, and how it works, I'll restate my case on the leaky and incomplete parts.
As I already mentioned, it's leaky because purify + computed (aka subscribe) still relies on the user of subscribe to understand ember's DK syntax and semantics. Thus the details of the underlying behavior (computed) leaks.
I also already stated my case about the abstraction being incomplete, but I'll try to be clearer this time. When I say "tries its best" I am mostly referring to this comment directly from the purify source.
Sure, you don't need this so you didn't make it work, but the fact remains that to no fault of your own, dependent keys can't be perfectly mapped to the properties the author of a computed property may want to use.
Which brings us to the actual point of my comment that you neglected to mention in your response: I don't understand the motivation and it isn't pragmatic.
Computed properties are member functions, so why bother making them pure in the first place? Computed property macros (which are in fact a documented feature of ember) generate member functions. This is the intended way to DRY out common computed properties (as further evidenced by the framework provided macros such as
alias
,collect
, etc.).And since computed properties are already a thing, and computed property macros are already a thing, and the remaining blemish of having to declare dependent keys is already a problem being addressed by the framework, there is no case to be made for introducing your own replacement being pragmatic.