generate static examples without api call [apm appveyor cdnjs clojars gem npm uptimerobot] #1740
Conversation
Generated by 🚫 dangerJS |
|
Hey @chris48s, thanks for picking this up. I really like this approach. Separating the formatting from the fetching is a smart way of separating concerns. For the current purpose it helps keep the preview badge appearances in sync. It also makes the badges more testable, more flexible, and probably easier to refactor. It enables smarter caching of service data, too. I took a long pass through another WIP on the NPM badges, which are among the more complicated badges, and tried to get a nice feeling between the subclasses and superclasses while trying to keep a reasonably tight grip on the indirection. My work is tackling adjacent concerns, but the abstraction lines up nicely. I wonder if we should push the abstraction up into BaseService. Almost all of our badges are doing fetch + render. That would allow the expression of these examples to be a little bit cleaner. I'll leave a few comments inline and open a PR with the work I did on NPM. |
services/cdnjs/cdnjs.js
Outdated
| @@ -7,6 +7,11 @@ const { version: versionColor} = require('../../lib/color-formatters'); | |||
|
|
|||
| module.exports = class Cdnjs extends BaseJsonService { | |||
| async handle({library}) { | |||
There was a problem hiding this comment.
Could you move this function declaration down? I'd prefer to declare the helper functions first, then the function that calls them. I've seen lots of work done with both styles, and find that way to be faster to read.
services/cdnjs/cdnjs.js
Outdated
|
|
||
| static formatBadge(version) { |
There was a problem hiding this comment.
I really enjoyed working with this abstraction in #1743, where I realized how well it mapped onto React's render() function, which at its purest, converts the passed-in properties into a DOM-like structure. To harmonize with that, I'd suggest we pass in an object with a single named property, e.g. static formatBadge({ version }).
There's one thing that feels slightly off about this abstraction, which is that you want the render/formatter function to take only the properties it needs, whereas the server response might either be organized in an inconvenient way, or contain a bunch of other stuff. In the npm badges, it worked beautifully to pass a copy of the response object, pruned down to the keys used by the badge. That would work here too. For a service which returned a more poorly organized response object, we'd probably want to clean up the schema before passing it to the render/formatter function.
What do you think about using React's name, render()?
services/cdnjs/cdnjs.js
Outdated
| @@ -43,7 +50,8 @@ module.exports = class Cdnjs extends BaseJsonService { | |||
| static get examples() { | |||
| return [ | |||
| { | |||
| previewUrl: 'jquery', | |||
| staticExample: this.formatBadge('1.5.2'), | |||
| exampleUrl: 'jquery', | |||
There was a problem hiding this comment.
If the render abstraction is pushed into the base class, we could put the properties here.
return [{
props: { version: '1.5.2' }, // formatBadge gets called automatically
exampleUrl: 'jquery',
...
}];Replacing the function call would make this easier to use with static properties when they come along.
services/base.js
Outdated
| @@ -82,23 +82,30 @@ class BaseService { | |||
| return '/' + [this.url.base, partialUrl].filter(Boolean).join('/'); | |||
| } | |||
|
|
|||
| static _makeStaticUrl(serviceData) { | |||
| const badgeData = this._makeBadgeData({}, serviceData); | |||
| const color = badgeData.colorscheme || badgeData.colorB; | |||
There was a problem hiding this comment.
Yay for having color support!
services/base.js
Outdated
| @@ -82,23 +82,30 @@ class BaseService { | |||
| return '/' + [this.url.base, partialUrl].filter(Boolean).join('/'); | |||
| } | |||
|
|
|||
| static _makeStaticUrl(serviceData) { | |||
There was a problem hiding this comment.
How about _renderToStaticBadgeUrl?
There was a problem hiding this comment.
It feels like this function and _makeFullUrl() are a pair of related functions and would benefit from similar names. If you're not a fan of _makeStaticUrl(), maybe _makeExampleUrl() or _makeStaticExampleUrl() would be better?
There was a problem hiding this comment.
_makeStaticExampleUrl works for me. There is also a function
shields/frontend/lib/badge-url.spec.js
Lines 39 to 43 in 66d444a
|
Cheers for having a look at this and providing comments. I like the improved terminology Since you've started generalising some of these ideas into the base classes in #1743 I think it is better to park this temporarily and I'll pick this up and rebase onto master once that is in. I think that will be better than trying to work on the 2 in parallel. |
… and [node] badges (#1743) When JSON responses come back, they are sometimes not in the format expected by the API. As a result we have a lot of defensive coding (expressions like `(data || {}).someProperty`) to avoid exceptions being thrown in badge processing. Often we rely on the `try` blocks that wrap so much of the badge-processing code, which catch all JavaScript exceptions and return some error result, usually **invalid**. The problem with this is that these `try` blocks catch all sorts of programmer errors too, so when we see **invalid** we don't know whether the API returned something unexpected, or we've made a mistake. We also spend a lot of time writing defensive tests around malformed responses, and creating and maintaining the defensive coding. A better solution is to validate the API responses using declarative contracts. Here the programmer says exactly what they expect from the API. That way, if the response isn't what we expect we can just say it's an **invalid json response**. And if our code then throws an exception, well that's our mistake; when we catch that we can call it a **shields internal error**. It's also less code and less error-prone. Over time we may be confident enough in the contracts that we won't need so many tests of malformed responses. The contract doesn't need to describe the entire response, only the part that's needed. Unknown keys can simply be dropped, preventing unvalidated parts of the response from creeping into the code. Checking what's in our response before calling values on it also makes our code more secure. I used Joi here, since we're already using it for testing. There may be another contracts library that's a better fit, though I think we could look at that later. Those changes are in base.js. The rest is a rewrite of the remaining NPM badges, including the extraction of an NpmBase class. Inspired by @chris48s's work in #1740, this class splits the service concerns into fetching, validation, transformation, and rendering. This is treated as a design pattern. See the PR discussion for more. There are two URL patterns, one which allows specifying a tag (used by e.g. the version badge `https://img.shields.io/npm/v/npm/next.svg`), and the other which does not accept a tag (e.g. the license badge `https://img.shields.io/npm/l/express.svg`). Subclasses like NpmLicense and NpmTypeDefinitions can specify the URL fragment, examples, the validation schema for the chunk of the package data they use, and a render function. The NpmVersion subclass uses a different endpoint, so it overrides the `handle` implementation from NpmBase. The remaining services using BaseJsonService are shimmed, so they will keep working after the changes.
|
Its taken a while to circle back to this one. I've updated just the one service for now as an example, but if we're happy with the tradeoffs here I'll have a look at updating the other new-style services to use static examples as well. |
|
As far as I'm concerned, this is golden. I do not think it's essential that the example badges be live. I think the tradeoff of a responsive UI is a great one, and that your solution does a great job mitigating the format skew by breaking out One thing it would be good to address before merging is adding a test to cover the new functionality in BaseService:
|
this makes the example less confusing and brings the style into line with others that define an exampleUri e.g: Travis, CircleCI, iTunes, etc
chris48s
changed the title
|
I've added tests and made a couple of minor tweaks. Also, having reflected on this, I think the issue described in the first post is alleviated if we make the examples more obviously 'fake' by using placeholder values (like we already do with some badges) instead of showing a wrong badge value for a real library/repo etc. I think that is less confusing Apart from that the main thing I've done is added static examples to existing badges. This should now cover everything we've refactored so far except for:
|
There was a problem hiding this comment.
This looks great.
One thing that comes to mind, now that most of the examples will be placeholders: sometimes it's not completely clear how to fill out the URL, and seeing a complete working example could help a lot with that. Do you think we should keep the other examples alongside, and render them in the UI as a list of example URLs (i.e. not as live badges)?
services/apm/apm.service.js
Outdated
| static get examples() { | ||
| return [ | ||
| { | ||
| exampleUrl: 'PACKAGE', |
There was a problem hiding this comment.
What do you think about using Sinatra-inspired syntax?
:package
This is a bit aspirational: I did a little work a while back with a library that took that kind of pattern and automatically converted it to regex, and also allowed you to substitute specific values into the pattern. That kind of thing would be really useful on the frontend.
It's also a format that's very recognizable, and attractive.
There was a problem hiding this comment.
Yeah. I also thought about {param} as that is used by a few frameworks, but I went with the PARAM style as we've done that in some other places. I'm down with:param.
| throw Error( | ||
| `Example for ${ | ||
| this.name | ||
| } is missing required previewUrl or staticExample` |
There was a problem hiding this comment.
Total aside: It would be helpful if instead of printing the classname, we printed the file where the class is defined. I wonder if we can do this by introspecting in the BaseService constructor.
There was a problem hiding this comment.
I did something similar, to a different end, in #1934.
There was a problem hiding this comment.
That's true, but I'm not sure we can use the same approach here. caller() returns null in this context
There was a problem hiding this comment.
Yea, that makes sense.
I was thinking there would be a way to snag it when the class was defined, though there’s no code from the superclass that runs at that time. The filename could be identified and associated with the service class at the time the service module is loaded. It’d be kind of dirty but might still be worth doing. I’ll give it some thought.
services/base.spec.js
Outdated
| @@ -15,17 +15,26 @@ const BaseService = require('./base') | |||
| require('../lib/register-chai-plugins.spec') | |||
|
|
|||
| class DummyService extends BaseService { | |||
| async handle({ namedParamA }, { queryParamA }) { | |||
| static render({ namedParamA, queryParamA }) { | |||
There was a problem hiding this comment.
Being on the reading side of this reminded me of @platan's suggestions about this test code made in #1589 (review).
I think I implemented all of his suggestions except this one. Would you mind making that change while you're here?
| }, | ||
| ] | ||
| } | ||
|
|
||
| static async render({ ratio }) { | ||
| static render({ ratio }) { |
|
Is there an heroku version up with the placeholders? |
Yeah I had a similar thought. I think it will be cluttered to show both in the 'list' view. What about if we show it in the modal dialog? |
|
@tooomm - I've just done a staging deploy at https://shields-staging-pr-1740.herokuapp.com/ if you want to have a look. Note this is likely to change in light of above comments. The list of services where I've changed the examples is in the PR title |
resolve conflicts on npm, gem, appveyor services
chris48s
changed the title
Isn't it already like that..?
Do you mean somewhere else? |
|
That's the spot I mean. When I triple-click the line, it selects "Example:" too:
|
|
Oh, perhaps a better option would be to wrap the link in |
|
Failures are the same issue as #1934 (comment) There's now some other badges that have been added/changed with dynamic examples that will want converting to static example. I think that's:
but the rest of the comments should be addressed now |
| previewUrl: 'dv/rails/stable', | ||
| exampleUrl: 'dv/rails/stable', | ||
| urlPattern: 'dv/:package/stable', | ||
| staticExample: this.render({ |
There was a problem hiding this comment.
Would it be better to pass tag: 'stable' into render?
There was a problem hiding this comment.
I've switched to generate the labels using out existing code for doing so in 1a5d94a instead of hard-coding it.
Side note: 'stable' is not a 'tag' in this situation. It is a special-case of version:
| previewUrl: 'npm/got', | ||
| exampleUrl: 'npm/got', | ||
| urlPattern: ':platform/:library', | ||
| staticExample: this.render({ dependentReposCount: '84000' }), |
There was a problem hiding this comment.
Can dependentReposCount be a number instead of a string? One more instance of this below.
services/npm/npm-version.service.js
Outdated
| previewUrl: 'npm', | ||
| exampleUrl: 'npm', | ||
| urlPattern: ':package', | ||
| staticExample: this.render({ tag: undefined, version: '6.3.0' }), |
There was a problem hiding this comment.
I think you can leave out tag: undefined and this will still work. (If not, that seems like it's probably a bug!)
|
After this is merged, let's split the APM service into separate files, and apply #1934 to it where possible. |
chris48s
changed the title
We're only cheating ourselves ;) |
|
This is live. |
Having chewed over the discussion in issue #1700 and drafted a few ideas, here is one possible solution:
If we use the service class internals to produce the example from an object and split the
handle()function into 2:_makeBadgeData()then it becomes easier to format an example value consistently with the real badge behviour without making any API call. If we seperate concerns correctly, this should minimise the scope for our examples to 'drift' or require a lot of maintenance.
In most cases, what we're doing fits cleanly into that pattern. In the cases where we are doing something more complicated, we can write a more complex
handle()function, elect not to return astaticExampleobject and just return a dynamicpreviewUrlas before. If we come up with a solution that addresses the common pattern, that allows us to make an improvement over where we are now.The one downside is that the examples obviously don't reflect reality. This will probably cause some confusion:
.. but this is a problem if we implement the suggestion in issue #1700 in any way. It is not inherent to this implementation.
Thoughts? I've picked one simple example here to demonstrate, but if this approach seems sensible I can have a look at updating the examples for other badges we've already refactored, adding tests, etc..