Automattic / gridicons

The WordPress.com icon set
http://automattic.github.io/gridicons/
GNU General Public License v2.0
110 stars 13 forks source link

Build: update svg-to-pdfkit to 0.1.8 (exact) #310

Closed folletto closed 3 years ago

folletto commented 3 years ago

Checking the problem raised in #309 it seems that for some reason the PDF build system broke again (past, see #258). I wasn't able to pinpoint the issue (maybe some specific package?), so the workaround as before is to pick an updated version, and reset the PDF generation.

This PR thus:

Testing:

  1. Try to do a clean pull (or delete node_modules) and check that on rebuild no files are marked as changed
  2. Run grunt and make sure again no files got changed
  3. Try to change any SVG source, and check that the files change as expected (git marks them as modified)
  4. Revert that SVG change, and check that the files are now rebuilt back (i.e. no more git modified files)
  5. Review that the story icon and the stats-alt-2 icon are the same as master branch
folletto commented 3 years ago

Problems:

folletto commented 3 years ago

For some reason, this currently still either generate from time to time empty PDFs, I can't find why sometimes works, sometimes doesn't

I fixed this, the problem is explained in #311, and this branch also has the same change now (6cf84f1b9fba8c7037718e590d01ba85ae64cf61).

folletto commented 3 years ago

I asked on the repository for PDFkit if they have any advice: https://github.com/foliojs/pdfkit/issues/1183

folletto commented 3 years ago

They have introduced at some point an ID field, which is generated from the timestamp like /ID [<b35ea4e0c3d8f9beab4bc67f5e6374a4> <b35ea4e0c3d8f9beab4bc67f5e6374a4>] (source). Some info here. This is bad news for us, as it means that every time it regenerates the PDF it gets changed.

Thanks to advice from Luiz Américo (https://github.com/foliojs/pdfkit/issues/1183) I was able to sort this out. The ID is based on all info fields, and CreationDate has to be set at constructor time, not after.

I tried to rebuild a few times, and it now seems reliably doing its job.

jasmussen commented 3 years ago

Took this for a spin, and after your latest push, I'm still seeing the ID of the PDF changed, but at least it changes it consistently to the same (i.e. running npm run build multiple times in a row results in the same):

Screenshot 2020-10-28 at 12 55 58

Here the diff is in copy pasteable text:

-/ID [<e6c22982f1d1b2baf3968fd38483fffe> <e6c22982f1d1b2baf3968fd38483fffe>]
+/ID [<a3d0a1f70210b663395e192e72506a75> <a3d0a1f70210b663395e192e72506a75>]

For me this isn't the end of the world if you need to merge this.

folletto commented 3 years ago

For me this isn't the end of the world if you need to merge this.

It's not in terms of output, but it would mean that anyone making a new icon would get 100+ changed files every time, so it makes tracking changes in the git history very hard. Or even checking that everything got built well.

The old version doesn't have this problem, and builds still well, so, this would def be a regression with no other benefit.

noahtallen commented 3 years ago

Closing since we aren't continuing with this change