Give the NPM package some love #2200

chris48s · 2018-10-20T15:48:33Z

Following on from the discussion in #1388 I've done a bit of work to whip the NPM package into shape so we can do a 2.0.0-beta1 release. There is one additional issue I think we should consider before publishing a 2.0.0 release:

Is it useful to force the user to make a choice of text measurer in order to generate a badge? Would it be better to be opinionated on this and provide a simpler public interface using either PDFKitTextMeasurer or QuickTextMeasurer (at least as a default)? As a point of reference, the CLI uses PDFKitTextMeasurer only.

we need this for publishing because npm will run the postinstall script after we install

shields-ci · 2018-10-20T15:49:41Z

	Warnings
⚠️	This PR added helper modules in `lib/` but not accompanying tests. Generally helper modules should have their own tests.

	Messages
📖	✨ Thanks for your contribution to Shields, @chris48s!
📖	Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

PyvesB

Looks good to me. I agree that we should hide PDFKitTextMeasurer and QuickTextMeasurer from the public interface. 👍

chris48s · 2018-10-23T17:02:10Z

I agree that we should hide PDFKitTextMeasurer and QuickTextMeasurer from the public interface.

OK - cheers. Given this is the only outstanding thing and will change the public interface and docs, lets tack that on to this PR.

Do you have a feel for whether it is sensible for the default (or possibly only) measurer to be QuickTextMeasurer or PDFKitTextMeasurer? We use QuickTextMeasurer on shields.io and PDFKitTextMeasurer in the CLI but I'm unclear on whether they are different for a good reason or because we changed it in one place and forgot to change it in the other.

Any thoughts?

PyvesB · 2018-10-23T17:36:58Z

After looking into this some more, as far as I can tell, these are both custom classes that we've written in text-measurer.js. QuickTextMeasurer uses PDFKitTextMeasurer but adds an additional layer of kerning/caching. Therefore I don't think that saying one is fast and the other accurate is correct.

The CLI targets one-off generations, therefore it doesn't make sense to add the caching capabilities, it would only slow things down.

If users use this as part of a Node module, they may or may not benefit from having caching depending on their use-case. However I think that they are most likely to write something that will call makeBadge several times and even if they don't, the performance impact of having to initialise and populate the cache will probably be negligible. Therefore I would go for QuickTextMeasurer as a default in the NPM module. We could also provide an alternative method or an optional withoutCache flag if users really want to override the default. 😉

chris48s · 2018-10-23T19:04:47Z

After looking into this some more, as far as I can tell, these are both custom classes that we've written in text-measurer.js. QuickTextMeasurer uses PDFKitTextMeasurer but adds an additional layer of kerning/caching. Therefore I don't think that saying one is fast and the other accurate is correct.

Aah yes I see you've used the classic "work out what's going on by actually reading the code" technique - cunning :)

My initial thought on this was that we should probably decouple the LRU cache from the badge generation (which would imply just using PDFKitTextMeasurer) but maybe that instinct is wrong.

We could also provide an alternative method or an optional withoutCache flag

Where possible I prefer the boolean condition to be positive. i.e: true should enable the thing and false should disable it rather than using a disableThing=true param) so I'd tend to think of it the other way round: call the param cache and default it to true.. but yeah, agreed.

I'll update this tomorrow. Cheers.

chris48s · 2018-10-24T18:40:43Z

How does this feel as a new public surface for the package?

PyvesB

The public interface is clean and simple, I like it. 👍

felixfbecker · 2018-10-25T19:09:53Z

README.md

-  badge(format, (svg, err) => {
-    // svg is a string containing your badge
-  })})
+const svg = bf.create(format)


So this is sync now?

Yep. As noted in #1388 (comment) its now synchronous

lib/gh-badges.js

paulmelnikow · 2018-10-27T17:14:39Z

Using PDFKitTextMeasurer will be faster for one-shot badge generation, since it only measures the two strings that are being used. On the other hand, QuickTextMeasurer builds the entire kerning table on startup, so it's slower. It would only be faster if you're generating hundreds of badges inside a single Node process, which I expect most people are not. (We should get rid of both of these things and just publish the lookup table, but that's a project for another day.)

paulmelnikow · 2018-10-27T17:17:10Z

lib/gh-badges.js

+const { PDFKitTextMeasurer, QuickTextMeasurer } = require('./text-measurer')
+
+class BadgeFactory {
+  constructor({ fontPath, fallbackFontPath, cache = true }) {


QuickTextMeasurer doesn't cache as much as it memoizes the most common inputs. Something like precomputeWidths would be more accurate.

Changed. Do you think this should default to true or false?

false, since I’d wager the most common use will be generating a single badge.

paulmelnikow · 2018-10-27T17:17:46Z

lib/gh-badges.js

+   * @param {string} format.colorB
+   * @param {string} format.format
+   * @param {string} format.template
+   * @return {string} Badge in SVG or JSON format


I'd love to switch this over to the schema we're using for new-style services, though that could definitely happen another day.

The DocBlock is helpful, because it allows editors/IDEs to show inline help, but we could definitely improve the validation. Lets save that for a minor release though

shields-deployment · 2018-10-30T18:34:10Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

chris48s added 6 commits October 20, 2018 16:04

define a public interface for NPM package

b383b22

move check-node-version to dependencies

2856968

we need this for publishing because npm will run the postinstall script after we install

add missing file to package

6fefdb7

update docs

7f2cdc1

bump version

f74ca81

add gh-badges option to issue template

e81f8c6

chris48s added the core Server, BaseService, GitHub auth label Oct 20, 2018

update package-lock

ecccbba

This was referenced Oct 20, 2018

Release gh-badges to npm #1388

Closed

use most permissive license color #2196

Merged

PyvesB previously approved these changes Oct 22, 2018

View reviewed changes

abstract text measuring from users/new public interface

38fa253

chris48s dismissed PyvesB’s stale review via 38fa253 October 24, 2018 18:39

PyvesB previously approved these changes Oct 24, 2018

View reviewed changes

felixfbecker reviewed Oct 25, 2018

View reviewed changes

add a DocBlock for BadgeFactory.create()

cb94899

chris48s dismissed PyvesB’s stale review via cb94899 October 25, 2018 20:08

PyvesB previously approved these changes Oct 27, 2018

View reviewed changes

paulmelnikow reviewed Oct 27, 2018

View reviewed changes

rename param

12e69db

chris48s dismissed PyvesB’s stale review via 12e69db October 28, 2018 11:03

chris48s added 3 commits October 28, 2018 16:01

Merge branch 'master' into issue1388

cdf54dc

change default to false

a6b41e9

explain text width computation in the docs

33a6c1c

paulmelnikow approved these changes Oct 29, 2018

View reviewed changes

Merge branch 'master' into issue1388

dbb4334

chris48s merged commit 6fc8744 into badges:master Oct 30, 2018

chris48s mentioned this pull request Nov 17, 2018

replace calls to makeBadge() with BadgeFactory.create() #2336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give the NPM package some love #2200

Give the NPM package some love #2200

chris48s commented Oct 20, 2018

shields-ci commented Oct 20, 2018 •

edited

Loading

PyvesB left a comment

chris48s commented Oct 23, 2018

PyvesB commented Oct 23, 2018 •

edited

Loading

chris48s commented Oct 23, 2018 •

edited

Loading

chris48s commented Oct 24, 2018

PyvesB left a comment

felixfbecker Oct 25, 2018

chris48s Oct 25, 2018

paulmelnikow commented Oct 27, 2018

paulmelnikow Oct 27, 2018

chris48s Oct 28, 2018

paulmelnikow Oct 28, 2018

paulmelnikow Oct 27, 2018

chris48s Oct 28, 2018

shields-deployment bot commented Oct 30, 2018

Give the NPM package some love #2200

Give the NPM package some love #2200

Conversation

chris48s commented Oct 20, 2018

shields-ci commented Oct 20, 2018 • edited Loading

PyvesB left a comment

Choose a reason for hiding this comment

chris48s commented Oct 23, 2018

PyvesB commented Oct 23, 2018 • edited Loading

chris48s commented Oct 23, 2018 • edited Loading

chris48s commented Oct 24, 2018

PyvesB left a comment

Choose a reason for hiding this comment

felixfbecker Oct 25, 2018

Choose a reason for hiding this comment

chris48s Oct 25, 2018

Choose a reason for hiding this comment

paulmelnikow commented Oct 27, 2018

paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

chris48s Oct 28, 2018

Choose a reason for hiding this comment

paulmelnikow Oct 28, 2018

Choose a reason for hiding this comment

paulmelnikow Oct 27, 2018

Choose a reason for hiding this comment

chris48s Oct 28, 2018

Choose a reason for hiding this comment

shields-deployment bot commented Oct 30, 2018

shields-ci commented Oct 20, 2018 •

edited

Loading

PyvesB commented Oct 23, 2018 •

edited

Loading

chris48s commented Oct 23, 2018 •

edited

Loading