There's a lot going on in this PR, though it's all interdependent, so the only way I can see to break it up into smaller pieces would be serially.
1. I completely refactored the functions for managing cache headers. These have been added to `services/cache-headers.js`, and in some ways set the stage for the rest of this PR.
- There are ample higher-level test of the functionality via `request-handler`. Refactoring these tests was deferred. Cache headers were previously dealt with in three places:
- `request-handler.js`, for the dynamic badges. This function now calls `setCacheHeaders`.
- `base-static.js`, for the static badges. This method now calls the wordy `serverHasBeenUpSinceResourceCached` and `setCacheHeadersForStaticResource`.
- The bitFlip badge in `server.js`. 👈 This is what set all this in motion. This badge has been refactored to a new-style service based on a new `NoncachingBaseService` which does not use the Shields in-memory cache that the dynamic badges user.
- I'm open to clearer names for `NoncachingBaseService`, which is kind of terrible. Absent alternatives, I wrote a short essay of clarification in the docstring. 😝
2. In the process of writing `NoncachingBaseService`, I discovered it takes several lines of code to instantiate and invoke a service. These would be duplicated in three or four places in production code, and in lots and lots of tests. I kept the line that goes from regex to namedParams (for reasons) and moved the rest into a static method called `invoke()`, which instantiates and invokes the service. This _replaced_ the instance method `invokeHandler`.
- I gently reworked the unit tests to use `invoke` instead of `invokeHandler`– generally for the better.
- I made a small change to `BaseStatic`. Now it invokes `handle()` async as the dynamic badges do. This way it could use `BaseService.invoke()`.
3. There was logic in `request-handler` for processing environment variables, validating them, and setting defaults. This could have been lifted whole-hog to `services/cache-headers.js`, though I didn't do that. Instead I moved it to `server-config.js`. Ideally `server-config` is the only module that should access `process.env`. This puts the defaults and config validation in one place, decouples the config schema from the entire rest of the application, and significantly simplifies our ability to test different configs, particularly on small units of code. (We were doing this well enough before in `request-handler.spec`, though it required mutating the environment, which was kludgy.) Some of the `request-handler` tests could be rewritten at a higher level, with lower-level data-driven tests directly against `cache-headers`.
It seems useful to accelerate #1961 even as the badge rewrites are still underway. This introduces a small amount of technical debt by hard-coding the static example, though continuing this work could allow us to eliminate the old ways of specifying examples. It seems like a decent tradeoff.
* Fix URL pattern for StackExchange Questions and Reputation per #2418
Url patterns have been changed back to their older legacy format.
Tests now run properly with this URL in addition to the examples
showing up on local.
* Make changes per review comments. Also change "q" to "t" parameter to match legacy url for questions
Per part of issue #2378, convert StackExchange service to the newer
BaseJson class. Refactor also to seperate StackExchange reputation and
questions tag info into new StackExchange subservice files
as the legacy class contained both which may be difficult to keep track
of in the future as the service continues to grow.
More tests have been added to make sure other StackExchange sites besides
StackOverflow will still work with the new badge setup.
* Allow to run service tests for remote shields instance
* Allow to run service tests for remote shields instance - doc
* Load testedServerUrl from env variable
* Load 'skipIntercepted' flag from env variable
JaCoCo uses "Line" as the type name for line coverage metric counter.
Added a condition to accept "Line" as a valid coverage stat label to
support JaCoCo style coverage reports.
See https://www.jacoco.org/jacoco/trunk/coverage/report.dtd for details
Avoids crashes in `server.spec.js` accompanied by this cryptic message:
```
Error: listen EADDRINUSE :::1111
at Object._errnoException (util.js:1022:11)
at _exceptionWithHostPort (util.js:1044:20)
at Camp.setupListenHandle [as _listen2] (net.js:1367:14)
at listenInCluster (net.js:1408:12)
at doListen (net.js:1517:7)
at _combinedTickCallback (internal/process/next_tick.js:141:11)
at process._tickDomainCallback (internal/process/next_tick.js:218:9)
```
e.g. https://circleci.com/gh/badges/shields/24094
These tests clearly should be refactored, though I’d like to get this fix in, in advance of a more involved refactor that shifts most of the responsibility away from this function. Maybe we can even eliminate at least one of these cases in the meantime.
There's a lot of demand for the Gitlab badges (#541) and the PR has been lingering, so I thought I'd start off one of the simple ones as a new-style service. This one is SVG-based, so it shouldn't require the API-token logic which could use some more testing and will require us to create an app and configure it on our server.
We don't have any validation in place for `queryParams`. Probably this should be added to BaseService, though for the time being I extracted a helper function.
Thanks to @LVMBDV for getting this work started in #1838!
Close#2334
To avoid merge conflicts, I've deferred removing the aliasing logic in `prepareExamples`. That whole function will be refactored momentarily, and there's also #2339 open.
Now that the static badge has been moved in #2284, next in line for cleaning out `server.js` is this “static badge, old format.” I imagine this route is _very, very old_. (I wouldn’t be surprised if it’s not used at all. I’d be curious to see some stats on that endpoint. If it's not regularly getting requests I could see dropping it.)
In the case of URLs which have permanently changed, an approach I’d like to try is issuing a 301 Redirect.
The benefit is that if a user pastes the URL into the address bar while they are previewing or editing it, the browser will replace the address with the corrected URL when it loads. I figure this will cause some people to update their URLs with no effort, simply because they previewed the badge in their browser, and others to change over, if they notice it.
We incur a slight cost, which is a second request. However many browsers cache the 301’s indefinitely, and we can set an effectively infinite cache duration so the CDN and most other downstream caches will keep them a long time. And handling the redirect is extremely cheap.
This is a nice way to preserve backward compatibility of old routes without having to complicate the new route, such as in the case of vso -> azure-devops. For maintenance purposes, the route that redirects can effectively be treated separately.
It’s also a nice, gentle, and confidence-inspiring way to signal that users should update their URLs.
We could generalize this code, though I think this is a good place to start. This route is tricky because it needs to be loaded last, complicating a reusable solution.
This continues the work from #2279, by allowing example badges to be specified using `namedParams`. Using an object makes it possible for us to display these in form fields down the line. (#701)
I've called this the "preferred" way, and labeled the other ways deprecated. I've also added some doc to the `examples` property in BaseService. Then I realized we had some doc in the tutorial, though I think it's fine to have a short version in the tutorial, and the gory detail in BaseService.
I've also added a `pattern` keyword, and made `urlPattern` an alias.
Closes#2050.
This picks up @RedSparr0w's work in #1802.
1. The handler for the static badge is moved into its own service with a synchronous handler. Avoiding an async call may make the static badges slightly faster, though it may be worth profiling this if it turns out we want asynchronous static badges in the future. If it doesn't make a performance difference we could make this handler `async` like the others.
2. Most of the custom static-badge logic is in a BaseStaticBadge base class.
3. Rewrite the static Gitter badge to use BaseStaticBadge.
4. A bit of minor cleanup in related functions.
This simplifies and further optimizes text-width computation by computing the entire width table in advance, and serializing it in the style of QuickTextMeasurer (#1390). This entirely removes the need for PDFKit at runtime. This has the advantage of fixing #1305 – more generally: producing the same result everywhere – without having to deploy a copy of Verdana.
The lifting is delegated to these three libraries, which are housed in a monorepo: https://github.com/metabolize/anafanafo
I'd be happy to move it into the badges org if folks want to collaborate on maintaining them.
QuickTextMeasurer took kerning pairs into account, whereas this implementation does not. I was thinking kerning would be a necessary refinement, though this seems to work well enough.
I dropped in a binary-search package to traverse the data structure, in part to conserve space. This causes a moderate performance regression, though there is ample room for improving on that: https://github.com/badges/shields/pull/2311#issuecomment-439182704
* move gh-badges files out of /lib
As far as possible, this is just moving files
around and updating paths however there are 2
functional changes in this commit:
- remove use of lib/register-chai-plugins.spec
in badge-cli.spec.js
- remove use of starRating()
in text-measurer.spec.js
* update service tests that use colorscheme.json
* split package.json in two
* clean up import
* don't hard-code path
* start a changelog
* put a license file in the package dir
* re-organise documentation 📚
* don't pack test files
* remove favicon from Makefile
* give package its own test command
* link the docs better in README
The NuGet badge examples are straggling in all-badge-examples. Rather than move them as is, I thought it made more sense to refactor the services and see if they could be generated. I didn't take that on here; this is a straight rewrite of the badges. The old implementations were fairly difficult to follow. The new implementations are complicated too, though I hope much more readable.
Though the NuGet behaviors could be consolidated into a single flag, I split `withTenant` and `withFeed` into separate flags, thinking naming the behaviors makes the implementations easier to understand. I defaulted these to true, thinking that really this is really a MyGet implementation which is generalized to NuGet. Though maybe it makes more sense to have the MyGet style as the default. Probably it doesn't matter much either way.
I added a helper class ServiceUrlBuilder to construct the Shields service URL. It's useful in this complex case where the URL must be built up conditionally. This might be useful in a couple other places.
I also wrote a new service to handle the Powershell badges. They've diverged a little bit from the Nuget v2. There's a bit of shared code which I factored out.
If the XML Nuget APIs are more reliable, we could consider switching everything else over to them, though for now I would like to get this merged and get #2078 fixed.
Fix#2078
The GitHub service family is the largest, and as yet untouched by our service rewrite. I thought I would start the process by tackling one service.
This pull request has a few things going on:
1. Rename pull-request-status to pull-request-check-state. We have another badge called pull request status. It seems like the checks are called one thing in the UI and another thing in the API, which is unfortunate. If other folks have strong feelings about the name, I’ll defer.
2. Move its tests and tighten up the syntax.
3. Move its badge examples including the doc string.
4. Add a new helper `errorMessagesFor` to use in the new services in place of `githubCheckErrorResponse`. It seems like we didn’t really use the `errorMessages` parameter to `githubCheckErrorResponse`, so I pared this down. I’m not sure if this is the function we’ll ultimately want, but it seems like a good place to start.
5. Pull fetch code I _know_ we use in other places into `github-common-fetch`. As in the PR I just opened for azure-devops, this takes a functional approach to the shared code, which is more direct, nimble, and easy to reason about than inheritance.
6. Create `GithubAuthService` which functions identically to BaseJsonService, except for one thing, which is that it uses the token pool. I accomplished this by adding a `_requestFetcher` property to BaseService, which is initialized to `sendAndCacheRequest` in the constructor, and can be overridden in subclasses. Since we weren’t using `_sendAndCacheRequest` directly except in BaseService and tests, I removed that property. I like this approach to patching in the GitHub auth because it’s very simple and creates no new API exposure. However, the way we’re doing the dependency injection feels a bit odd. Maybe the eventual refactor of request-handler would be a godo time to revisit this.
The GitHub requests go through many, many layers of indirection at this point. Later on it would be good to shave some of these off, perhaps once the legacy GitHub services have been converted, or when all the services are done and we can take another look at the base service hierarchy. The work in #2021 and #1205 is also related.
The term “url” is overloaded in services, to refer to the Shields route and also the API URL. Calling the Shields URL a “route” is on the whole more descriptive, and makes it clearer and more obvious which one of these we’re talking about. It’s a small thing, though seems like an improvement.
We have a few functions called `buildUrl`. I’ve renamed them to `buildRoute` when they refer to routes, and left them as `buildUrl` when they refer to API URLs.
I included a minor style tweak and some formatting cleanup in `TUTORIAL.md`.
