Skip to content
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

move [static-badge] to own service & add test #1802

Closed
wants to merge 9 commits into from

Conversation

RedSparr0w
Copy link
Member

Move the static badge to its own service module,
Add tests.

I'm unsure how to add the custom headers back such as these though:

ask.res.setHeader('Cache-Control', 'max-age=' + cacheDuration);
ask.res.setHeader('Last-Modified', serverStartTime.toGMTString());

@shields-ci
Copy link

shields-ci commented Jul 23, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @RedSparr0w!

Generated by 🚫 dangerJS

@RedSparr0w
Copy link
Member Author

I can't set it so the label/message accepts being empty,
I've tried the following:

    return {
      label: label || '',
      message: message || '',
      color
    };
  static get defaultBadgeData() {
    return { label: '', message: '' };
  }

And those do not seem to help.

Any ideas?

@paulmelnikow
Copy link
Member

Phew, thanks for taking this on!

Re the custom headers and the caching:

Because the static badge is one-of-a-kind, I think we should deal with the differences in the static badge's service class rather than building a bunch of extra features into BaseService.

So I'd suggest we give StaticBadge its own implementation of register(). That method is responsible for invoking camp.route. When you invoke it you can provide your own callback like we do in server.js and then you'll have access to the ask parameter. Plus it'll allow us to maintain the same cache behavior. The handleRequest call in the second argument to camp.route is the same as cache in server.js. You'll notice there's no call to cache in for the static badge; another reason to override register().

camp.route(this._regex, handleRequest({

This code translates from named params + query params to serviceData:

shields/services/base.js

Lines 209 to 214 in f5fe85c

const namedParams = this._namedParamsForMatch(match);
const serviceInstance = new ServiceClass({
sendAndCacheRequest: request.asPromise,
}, { handleInternalErrors });
const serviceData = await serviceInstance.invokeHandler(namedParams, queryParams);
const badgeData = this._makeBadgeData(queryParams, serviceData);

Since you don't need sendAndCacheRequest you could pass {} for context.

Re the empty message, I'd suggest we replace this code:

shields/services/base.js

Lines 187 to 190 in f5fe85c

text: [
overrideLabel || serviceLabel || defaultLabel || this.category,
serviceMessage || 'n/a',
],

with a function like this:

function coalesce(...candidates) {
  return candidates.find(c => typeof c === 'string');
}

console.log(coalesce('', 'foo', ''));
console.log(coalesce('asdf', 'foo', ''));
console.log(coalesce(null, '', 'bar'));
console.log(coalesce(undefined, '', 'bar'));

I'm pretty sure accepting an empty string anywhere along that chain will do the right thing. We aren't in the habit of returning an empty string as a kind of null.

@paulmelnikow
Copy link
Member

@RedSparr0w Would you like me to pick up the changes to register()? I'm happy to hold off if you'd like to do it. 😁

@RedSparr0w
Copy link
Member Author

If you could that would be great! 👍

@paulmelnikow paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Aug 1, 2018
@paulmelnikow
Copy link
Member

Mind merging master into this branch when you have a moment? I wrote some steps here: #1866 (comment)

@paulmelnikow
Copy link
Member

Reopened as #2284!

paulmelnikow added a commit that referenced this pull request Nov 17, 2018
…2284)

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.
@RedSparr0w RedSparr0w deleted the static-service branch January 21, 2019 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants