ui: HTTPAdapter queryLeader refactor #6386
Conversation
ui-v2/app/adapters/node.js
Outdated
| return adapter.requestForQueryLeader(request, serialized, unserialized); | ||
| }, | ||
| function(serializer, response, serialized, unserialized) { | ||
| return serializer.respondForQueryLeader(response, serialized, unserialized); |
There was a problem hiding this comment.
It’s a bit confusing to me that this is called respondFor… but a response is passed in. The actual function definition calls it respond instead. Maybe it should be responseForQueryLeader, as it seems to me that it’s generating a response, not actually responding? But maybe I’m way off hehe
Also, the function doesn’t look like it uses the third parameter, is there a reason to include it in this call?
There was a problem hiding this comment.
Hmmm yeah, thinking back I wanted it to match up with requestFor..., but I suppose I was thinking of it as a verb rather than a noun - I kinda tried to make this look like the old urlFor... things which uses nouns, so maybe these should be thought of as nouns also as you suggest here. Let me take another look at it, the reasons why (if there were any) might have got lost in the refactor.
There was a problem hiding this comment.
Not looked in detail yet buck, but just realized something else here which might be more 'correct' sounding. request and response are actually functions, that when called 'do' the request things, or the response things. So I'm considering the option of changing the name of the argument to respond here. As its a function, a verb usually fits better. Just wanted to P.S. that on here, I'm gonna take a closer look now.
There was a problem hiding this comment.
Hey, so, I went with changing the arg to be called respond, which as its a function looks more 'function-y' - lemme know what you think.
Also, the function doesn’t look like it uses the third parameter, is there a reason to include it in this call?
I generally keep unused arguments in there, mainly for basic documentation (somebody else might come along and see they have unserialized at their disposal within the function's scope even though it wasn't used previously) and for consistency.
Unused arguments will get uglified out in the build also
| // This response is just an ip:port like `"10.0.0.1:8000"` | ||
| // split it and make it look like a `C`onsul.`R`esponse | ||
| // popping off the end for ports should cover us for IPv6 addresses | ||
| // as we should always get a `address:port` or `[a:dd:re:ss]:port` combo |
There was a problem hiding this comment.
Maybe it would be helpful to show example input and output here? I see the output but I’m not sure what input it’s operating on.
There was a problem hiding this comment.
Just incase, [Outdated] doesn't show in the GH ui in certain places as a code comment has been added for this a few lines under here to address this comment.
1. Upgrade consul-api-double again 2. Fix CONSUL_PROXY_COUNT again
This wording fits better with the fact that its a function that does stuff
johncowen
force-pushed
the
feature/ui-http-adapter-query-leader
branch
from
df63c7d to
875e839
Compare
store.queryLeader now appears twice after that merge, we remove the duplicate here
These values are generally merged together, and consul-api-double now has 2 meta values for services. Add another additional meta value here for testing/mocking purposes.
|
I've added a couple of commits on the end of here that were required as a result of merging and rebasing this to pull it up to date with |
As the HTTPAdapter work (see #5637) has been long running, we've since added functionality to figure out the leader of the consul datacenter, which uses the status API (see #6265).
This PR migrates this work over to the new HTTPAdapter, we also add a couple of extra tests here.
The related changes commit also happen elsewhere, and are 'incidental' changes to pretty much tie up all the loose ends of this run of work whilst keeping the ember upgrade as a separate PR.
As a side note: You can really see the difference in this diff in complication with adding a new API call here between the old way and the new way 🎉