Template literal templates #4459

chris48s · 2019-12-23T19:57:13Z

This turned into an absolute beast, but I think we're finally good for review. I suspect the review process for this one is going to be pretty involved too. I'll mark up the diff with a couple of specific comments, but here's some high level notes:

The core purpose of this PR is to remove use of the doT template library and move to generating SVG output using javascript template literals. The rendered SVG should be unchanged in all cases.
Paul started this work in Template literal templates #3637 and then it stalled. I've pretty much squashed all the work from that PR into 058fa48 (apart from the bit we already merged in Generate JSON badges without using a template [GithubSearch] #3395) and then iterated from there.
This PR targets the v3-dev branch, not master. Getting this merged will unblock the rest of the work we need to do on V3 so hopefully we'll be able to get the rest of the breaking changes done and merged to master once we can draw a line under this.
A secondary consequence of this work is we needed to re-consider our approach to SVG optimisation. Roughly in line with Paul's comments in Template literal templates #3637 (comment) I decided to drop SVGO at the library layer and mostly manually implement the optimisations (running SVGO won't be an exact no-op). See 286636d , 8e62a86 , b877bc9 and 2f62ab7 . The exception to this is that sometimes SVGO replaces a <rect> with a <path> element if the <path> takes up less characters. I haven't bothered implementing this optimisation. This does mean that we'll serve a few extra characters for every badge (the re-generated snapshots in ed24253 give an idea of the impact of this), but I think the tradeoff is OK.
I decided that flat, flat-square and plastic are all sufficiently similar that they are best thought of as slight variations of the same thing so they are implemented as child classes of a base class. This allows most of the implementation to be shared. for-the-badge and social styles were so different that they don't really shoehorn into that pattern, so those two are just implemented as completely bespoke render functions, as opposed to as subclasses of Badge.
Given this PR is really ripping out the guts of the core rendering logic and starting again, this PR could use review from more than one of the core maintainers.
There's a staging deploy at https://shields-staging-pr-4459.herokuapp.com/
Things that would be particularly useful to get feedback on:
- Are we escaping all the things that need escaping - have I missed any calls to escapeXml()?
- Are there any cases or situations I've forgotten to test?

Closes #2428
Refs #3587

shields-ci · 2019-12-23T20:00:47Z

	Warnings
⚠️	This PR targets `v3-dev` - It is likely that the target branch should be `master`

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

Generated by 🚫 dangerJS against 34db0f2

get flat, flat-square and plastic styles working + factor out common code to a base class

lgtm-com · 2019-12-23T20:09:11Z

This pull request introduces 3 alerts when merging 4e7c4d7 into c05394d - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class
1 for Useless assignment to local variable

lgtm-com · 2019-12-28T22:08:08Z

This pull request introduces 2 alerts when merging 7eae7df into c05394d - view on LGTM.com

new alerts:

1 for Useless assignment to local variable
1 for Unused variable, import, function or class

lgtm-com · 2019-12-29T21:01:31Z

This pull request introduces 1 alert when merging c2bf4fd into c05394d - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

this is the default

This accounts for several things: 1. Changes in one-char variable names 2. Removed some whitespace 3. I've dropped the <rect> to <path> optimisation

lgtm-com · 2019-12-30T19:53:05Z

This pull request introduces 1 alert when merging 5bba06f into c05394d - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

__snapshots__/badge-renderers.spec.js

gh-badges/lib/make-badge.spec.js

labelColor is always true anyway now we've given the param a default

…et here)

at this layer of the application, a logo should already be a base64 encoded string. `logo: 'javascript'` renders <image x="5" y="3" width="14" height="14" xlink:href="javascript"/> which isn't really what we want to test Passing logoColor to makeBadge() is completely meaningless

gh-badges/lib/badge-renderers.js

paulmelnikow · 2020-01-08T16:37:41Z

gh-badges/lib/make-badge.js

-    rightWidth++
+  const methodName = camelcase(template)
+  if (!(methodName in badgeRenderers)) {
+    throw new Error(`Unknown template: '${template}'`)


Not blocking, though at some point this should be consolidated with the style validation logicin gh-badges/lib/index.js.

paulmelnikow · 2020-01-08T16:38:52Z

This is looking good!

additional changes made since this review

chris48s · 2020-01-08T20:02:47Z

Something I haven't done is look at a bunch of badges. What do you think about restoring the test page

Yep. Good call - that's now at https://shields-staging-pr-4459.herokuapp.com/dev/styles

Having looked at all those, I spotted another issue which I've fixed in 34db0f2

paulmelnikow

🎉

chris48s · 2020-01-09T20:02:42Z

Cheers for review all :)

I've merged this onto the v3-dev branch. Over the weekend I'll review where I got to with the v3 roadmap: what else needs to go into v3.0/what can wait till v3.1 and pick up the rest of this. I'd like to get to a stage where we can ship this sooner rather than later.

- Remove use of the doT template library and move to generating SVG output using javascript template literals. - Drop SVGO and mostly manually implement the optimisations. - Add a bunch more tests Co-authored-by: Paul Melnikow <github@paulmelnikow.com>

* Validate input to BadgeFactory.create() (#3875) * validate input to create() * remove deprecated properties (#3881) * remove BadgeFactory class (#3884) * Template literal templates (#4459) - Remove use of the doT template library and move to generating SVG output using javascript template literals. - Drop SVGO and mostly manually implement the optimisations. - Add a bunch more tests Co-authored-by: Paul Melnikow <github@paulmelnikow.com> * drop raster support in package CLI (#4523) * drop raster support in package CLI * update docs * rename gh-badges package to badge-maker * rename gh-badges dir to badge-maker * update relative imports and other refs to in parent dir 'gh-badges' --> 'badge-maker' * update snyk service tests This change is only tangentially related We've used the shields repo as an example for these tests so moving files around in our repo has a knock-on effect on them * add missing type hints to dev style page * write the changelog/migration guide for v3 * use extension in README CLI example * update CLI help whoops - missed this in #4523 * bump version * update for self-hosting users * README updates * drop .format param from CLI, always output SVG * Change text[] to label and message, Remove JSON output - Change text[] to label and message - Fix message only badge - Remove JSON output format - Update the docs * update package-lock * rename 'template' to 'style' * handle invalid styles in coalesceBadge * ensure makeBadge is passed a string for template in coalesceBadge() issue #4925 * fix (logo/no label text/label color specified) case issue #4926 * add example of (logo/no label text/label color specified) to style debug page * update type defs * padding fix for FTB style Co-authored-by: Paul Melnikow <github@paulmelnikow.com>

chris48s and others added 6 commits December 23, 2019 20:01

add a bunch more snapshot tests

fcf6c68

WIP: this is _roughly_ where Paul got to in badges#3637

058fa48

get flat/flat-square/plastic working

591ce21

get flat, flat-square and plastic styles working + factor out common code to a base class

get FTB working

a616528

escape logo

1944de9

fix social logo rendering

4e7c4d7

chris48s force-pushed the template-literals2 branch from e944eee to 4e7c4d7 Compare December 23, 2019 20:02

fix badges with only message and logo, cleanup in FTB

7eae7df

chris48s added 2 commits December 29, 2019 17:14

fix link rendering code

db6817e

fix social badge link rendering, tidy up

c2bf4fd

chris48s added 8 commits December 30, 2019 13:29

manually optimise: scale(0.1) --> scale(.1)

286636d

manually optimise: remove lengthAdjust="spacing"

8e62a86

this is the default

manually optimise: one character var names

b877bc9

manually optimise: whitespace

2f62ab7

var names cleanup

be11eba

Re-generate snapshots

ed24253

This accounts for several things: 1. Changes in one-char variable names 2. Removed some whitespace 3. I've dropped the <rect> to <path> optimisation

remove dot and SVGO from package deps

9bbb2a4

add massive pile of tests

5bba06f

remove pointless assignment

a1eb9cb

shields-cd temporarily deployed to shields-staging-pr-4459 December 30, 2019 20:15 Inactive

chris48s added blocker PRs and epics which block other work npm-package Badge generation and badge templates labels Dec 30, 2019

chris48s commented Dec 30, 2019

View reviewed changes

__snapshots__/badge-renderers.spec.js Outdated Show resolved Hide resolved

chris48s commented Dec 30, 2019

View reviewed changes

gh-badges/lib/make-badge.spec.js Outdated Show resolved Hide resolved

chris48s mentioned this pull request Dec 30, 2019

Template literal templates #3637

Closed

chris48s added 5 commits January 5, 2020 13:38

simplify labelColor computation

ccdc468

labelColor is always true anyway now we've given the param a default

remove unused param (we've already applied logoColor by the time we g…

0db6b52

…et here)

move all the snapshot tests out a layer

62529b2

re-generate test snapshots

19224e5

shields-cd temporarily deployed to shields-staging-pr-4459 January 5, 2020 15:18 Inactive

throw with different error message

1a83138

shields-cd temporarily deployed to shields-staging-pr-4459 January 5, 2020 18:59 Inactive

RedSparr0w reviewed Jan 6, 2020

View reviewed changes

gh-badges/lib/badge-renderers.js Show resolved Hide resolved

paulmelnikow previously approved these changes Jan 8, 2020

View reviewed changes

chris48s and others added 3 commits January 8, 2020 19:40

express logo X and Y more clearly, fix plastic logo placement

c5ef23a

Add test page

d5fe0e6

fix FTB style with logo but no label

34db0f2

shields-cd deployed to shields-staging-pr-4459 January 8, 2020 19:57 View deployment

paulmelnikow approved these changes Jan 8, 2020

View reviewed changes

chris48s merged commit 198aeaf into badges:v3-dev Jan 9, 2020

chris48s deleted the template-literals2 branch January 9, 2020 19:59

paulmelnikow mentioned this pull request Jan 9, 2020

NPM audit vulnerability on dependency of gh-badges package #3480

Closed

3 tasks

PyvesB mentioned this pull request Feb 23, 2020

Remove the popout badge formats? #3635

Closed

chris48s mentioned this pull request Mar 8, 2020

📦 version 3 #4756

Merged

paulmelnikow mentioned this pull request May 5, 2020

Fix rendering in Firefox on Windows #5038

Merged

chris48s mentioned this pull request Oct 16, 2020

A few straight refactors in badge-renderers #5716

Merged

chris48s mentioned this pull request Oct 30, 2020

Make for-the-badge letter spacing more predictable, and rewrite layout logic #5754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template literal templates #4459

Template literal templates #4459

chris48s commented Dec 23, 2019 •

edited

Loading

shields-ci commented Dec 23, 2019 •

edited

Loading

lgtm-com bot commented Dec 23, 2019

lgtm-com bot commented Dec 28, 2019

lgtm-com bot commented Dec 29, 2019

lgtm-com bot commented Dec 30, 2019

paulmelnikow Jan 8, 2020

paulmelnikow commented Jan 8, 2020

chris48s commented Jan 8, 2020

paulmelnikow left a comment

chris48s commented Jan 9, 2020

Template literal templates #4459

Template literal templates #4459

Conversation

chris48s commented Dec 23, 2019 • edited Loading

shields-ci commented Dec 23, 2019 • edited Loading

lgtm-com bot commented Dec 23, 2019

lgtm-com bot commented Dec 28, 2019

lgtm-com bot commented Dec 29, 2019

lgtm-com bot commented Dec 30, 2019

paulmelnikow Jan 8, 2020

Choose a reason for hiding this comment

paulmelnikow commented Jan 8, 2020

chris48s commented Jan 8, 2020

paulmelnikow left a comment

Choose a reason for hiding this comment

chris48s commented Jan 9, 2020

chris48s commented Dec 23, 2019 •

edited

Loading

shields-ci commented Dec 23, 2019 •

edited

Loading