Page MenuHomePhabricator

Move MobileFrontend/Minerva's svg_check.sh script into a reusable, separate library
Closed, DuplicatePublic

Description

AIUI the script runs svgo to see if any of the svgs can benefit from compression. This seems like a useful check for other extensions and core too, and we should turn it into something more reusable, maybe a grunt plugin?

Event Timeline

Legoktm created this task.Oct 31 2017, 3:30 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 31 2017, 3:30 AM
Jdlrobson moved this task from Backlog to New adventures on the MinervaNeue board.Oct 31 2017, 5:44 PM

I agree, this would be very useful. We considered this a while back, but couldn't work out how to make this happen with our CI setup.

The svg_check.sh runs the local version of svgo as installed by npm with some specific parameters. If the output differs it assumes it hasn't been run and fails. It doesn't fix the problem - the user needs to manually run the same command and commit those files.

For this reason it's been tricky to encapsulate this in something generic - there are two parts - the thing that checks and the thing that fixes. It requires a team to be disciplined and use a pre-commit hook or at least know about the magic needed to compress the SVGs and these need to be in sync. Do we have any similar tools that do this sort of thing that we could get some ideas from?

Is anyone in release engineering available to help support exploring this?

Jdlrobson triaged this task as Medium priority.Oct 31 2017, 10:38 PM

You can just have two commands:

  • svg-check check images/ which checks and exits with a non-zero status code if some svgs aren't optimized
  • svg-check fix images/ which is just a wrapper around svgo and optimizes everything that people will then commit

As long as it gets run by "npm test" there's nothing in the CI stack that needs to be changed - it can all be handled at the repository level.

This matches the same pattern that eslint, phpcs, minus-x, etc. use.

Ok I see what you mean. It would still be opt in but handled by a single node module.

For performance we only check svgs in the current commit so we'd still need to shell out to git and know which folders to scan or scan everything at added cost.

This would help @VolkerE out a lot.

bmansurov added a subscriber: bmansurov.

Looks like the task is clear. Moving along.

Jdlrobson claimed this task.Nov 2 2017, 5:46 PM
Jdlrobson added a project: User-Jdlrobson.

I'll explore a proof of concept as recall this is not as easy as it looks.

matmarex removed a subscriber: matmarex.Nov 4 2017, 11:01 PM

I was talking to @Volker_E recently about this and this would also make his life a lot easier.

After re-thinking this option I'm not sure about the use case any more? SVG optimization is a little delicate and needs QA. Nevertheless from my work over at T178867 I assume that with reasonable defaults we can make SVGO work in 9 of 10 or our products without distorted images by default.
https://github.com/wikimedia/WikimediaUI-Style-Guide/pull/54 builts on top of grunt-svgmin and that could be exemplified as another default linter similar to our others.

What would be the use case for the shell script as grunt plugin aside of above?

What would be the use case for the shell script as grunt plugin aside of above?

The grunt plugin needs to be able to detect when it hasn't been run and throw an error code when this happens inside npm test. If we can make this grunt plugin do that, we can just use it out of the box and people can add it to npm test

svgminification is very different to linting. Linting is read only, svg minification reads and then writes.

We'd want the svg minification via grunt to fail if run inside Jenkins.. I'm guessing that's just a case of updating the integration test so that it throws an error if you try and write to the resources folder (when no minification is needed no write should be needed)? Can this be done?

Jdlrobson moved this task from Inbox to Tracking on the User-Jdlrobson board.Nov 13 2017, 9:55 PM

@Jdlrobson Sure, the boundaries between linting and minification are blurry in the case of SVG though and I'm not sure if a two tools approach is making our lives better. Can't speak to the rightful question about the integration test.

Niedzielski added a subscriber: Niedzielski.
Volker_E added a comment.EditedJun 6 2019, 12:31 AM

Repeating what I've said before, coming from https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/514631/
I don't think it's very reasonable to leave minification in, it's hard to find non-standardized SVGs and it negatively affects readability.
Given it's 27 files currently in MF, the savings are in sub-1-KB range.

greg added a subscriber: greg.

(afaict this doesn't effect releng, let me know if I'm wrong)

greg removed a subscriber: greg.Jul 18 2019, 4:41 PM

Change 570518 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/MinervaNeue@master] Update '.svgo.yml' with pretty print rule and re-crush all icons

https://gerrit.wikimedia.org/r/570518

Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptFeb 11 2020, 4:28 AM

Change 570519 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/extensions/MobileFrontend@master] Update '.svgo.yml' with pretty print rule and re-crush all icons

https://gerrit.wikimedia.org/r/570519

Change 570518 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Update '.svgo.yml' with pretty print rule and re-crush all icons

https://gerrit.wikimedia.org/r/570518

Change 570519 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Update '.svgo.yml' with pretty print rule and re-crush all icons

https://gerrit.wikimedia.org/r/570519