Move JSON rendering from badge-maker to server [*****]

[paulmelnikow]

1. Move normalization from "internal" makeBadge() into a _clean() function which is privately exported by badge-maker. It's necessary that the server can access this functionality to do the JSON rendering. (Alternatively we could remove the underscore to make this part of the public API. However this may not be necessary, as I think it will only be needed for Shields.)
2. Some changes to help with Depend on badge-maker's public interface instead of relative imports #4950:
   1. Bring the "internal" badge-maker schema more in line with the badge-maker public schema.
   2. Bring the output of coalesceBadge() more in line with the badge-maker public schema.
   3. Since the badge-maker public interface requires strings, the package internals should assume a string. Accordingly, move numeric coercion from the the package internals to the server.

Closes #4948

[shields-ci]

	Warnings
⚠️	This PR modified the server but none of its tests. That's okay so long as it's refactoring existing code.

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS against 0a68459

[paulmelnikow]

Failures seem unrelated, except that one triggered an error in an untested code path in the request handler, which I fixed in 0a68459.

[chris48s]

I'll have a look at this one over the weekend

[chris48s]

There's some sensible stuff going on here e.g: moving some of the normalize/default logic out of make-badge.makeBadge() into _clean and using more consistent terminology in the input params for make-badge.makeBadge().

Having seen the implementation: If you consider that the near-future goal is that we generate our own (SVG) badges via index.makeBadge() (and hence everything will go through _validate() and _clean() implicitly), do you reckon exporting an undocumented function is worse than exposing an undocumented format="json" param or depending on a relative import for the purpose of json output?

They all represent a sort of "backdoor", or a way in which we're not going to eat our own dogfood, so if we're going to do one of those things we might as well pick the one that requires us to do the least acrobatics elsewhere in the codebase achieve it, if you see what I mean.

[chris48s]

For a message only badge, running

cleaned.label = `${cleaned.label}`.trim()

would result in the label property being the string literal "undefined". This is not what we want.

[paulmelnikow]

Indeed. I'll fix this up tomorrow.

[paulmelnikow]

Having seen the implementation: If you consider that the near-future goal is that we generate our own (SVG) badges via index.makeBadge() (and hence everything will go through _validate() and _clean() implicitly), do you reckon exporting an undocumented function is worse than exposing an undocumented format="json" param or depending on a relative import for the purpose of json output?

They all represent a sort of "backdoor", or a way in which we're not going to eat our own dogfood, so if we're going to do one of those things we might as well pick the one that requires us to do the least acrobatics elsewhere in the codebase achieve it, if you see what I mean.

The server needs to access the normalization capability, though not the validation code, of the badge maker, separately from invoking makeBadge(), so it can emit normalized values in the JSON response.

I suspect we should export a public normalize() function for this. It's unlikely anyone other than Shields will use it, but it's still a reaonsably logical part of the API. It feels cleaner than having a { format: json } parameter, particularly since we make these weird legacy modifications to the schema in the server anyway (and I definitely don't think we should have those in the package).

Maybe this function should assume the data is valid, or maybe it should validate it, not sure. I haven't thought carefully about what is validation vs. what is normalization. We could tackle that now, or defer it to a follow-on. Deferring might be the thing to do since we want to release 3.0.0… if it's private and undocumented, then we can make it public in 3.1.0.

Thoughts?

[chris48s]

The server needs to access the normalization capability, though not the validation code, of the badge maker, separately from invoking makeBadge(), so it can emit normalized values in the JSON response.

If the objective is to move JSON.stringify() from one place to another, yeah. Just to take a step back though..

Once we do #4947 #4949 and #4950 the code in server.js / legacy-request-handler.js becomes

const { makeBadge } = require('badge-maker')
//...
makeBadge(badgeData)

..at which point everything for the SVG request is getting processed by _clean() and _validate() anyway. We shouldn't be passing anything to makeBadge() that doesn't pass _validate() - if we are, that's something we need to fix.

In principle, I agree that not exposing format='json' as an undocumented option on makeBadge() is conceptually nicer if it gives us a completely clean interface boundary, but if we trade off an undocumented "secret" param for an undocumented "secret" function, maybe the lesser evil is just to leave the JSON.stringify() in the package and go with and undocumented param if that means the server code is like:

makeBadge({...badgeData, format='json'})

instead of doing the whole makeBadgeOrJson() dance?

[paulmelnikow]

Having thought about this a while, I feel like it's pretty fine for the package to have non-secret validate() and normalize() functions.

[paulmelnikow]

I've picked a couple bits of this out. I'm thinking I'll close it for now, and wait to address the rest when we're ready to dogfood our own package.