use fetch/render pattern in [apm appveyor cdnjs clojars gem] #1901
Conversation
this case is covered in base class tests added in badges#1890
services/apm/apm.service.js
Outdated
| @@ -41,11 +41,13 @@ class BaseAPMService extends BaseJsonService { | |||
| } | |||
|
|
|||
| class APMDownloads extends BaseAPMService { | |||
| static render(downloads) { | |||
There was a problem hiding this comment.
Could we pass { downloads } here instead? I prefer that pattern as it reads better on the calling side, and grows well.
There was a problem hiding this comment.
Shall I switch all the fetch() and render() methods to use the object destructuring syntax, or just the render()s ? I don't have particularly strong opinions on this, I'm just slightly unsure what the tradeoff is in this situation..
There was a problem hiding this comment.
I think named parameters are great. They make the calling code self-documenting, and allow functions to grow organically with minimal impact at the callsite. As the number of parameters grows past one, naming them reduces the risk of passing the wrong value to a function. Of course JS doesn't have named parameters, but destructuring provides basically the same thing.
So, I'd suggest doing both!
Once this is in, I will pick up #1740 again