HatScripts / circle-flags

A collection of 400+ minimal circular SVG country, state and language flags
https://hatscripts.github.io/circle-flags
MIT License
985 stars 256 forks source link

More explicit svgo version #99

Open waldyrious opened 1 year ago

waldyrious commented 1 year ago

Moving a discussion started in c9eed380a280b3b85c172266d32e9595f4c67aa8 into an actual issue, to make it more trackable :) Repeating what I said there:

@HatScripts I think it's better to mention a specific version or version range that's been tested to work, in case they introduce breaking changes that invalidate this statement. E.g. https://github.com/HatScripts/circle-flags/issues/93.

@HatScripts, would you mind copying your response below, so it gets properly attributed to you? Then we can continue the discussion here.

HatScripts commented 1 year ago

@waldyrious That's a good point. But doesn't the "or newer" part kind of undermine what you are suggesting?

waldyrious commented 1 year ago

Yes, I agree that the previous wording was too vague too. That's why I suggested a version range. However, that could be a little awkward to track manually; perhaps we should add svgo as a devdependency in package.json, with an explicit version range, and add CI to test both ends of that version specification.

Alternatively, as a simpler method, we could simply mention the specific version that has been manually tested to work, and using anything else would be at the responsibility of the contributor.

HatScripts commented 1 year ago

I'm afraid I'm not all that familiar with Node.js, package.json, devDependencies, CI, etc.

If we want to keep things simple, how about including something like this in the README under the Contributing section?

To contribute, you need to have v3.0.2 of svgo installed:

npm -g install svgo@3.0.2
waldyrious commented 1 year ago

Yeah, that sounds good! To be honest the automated CI setup would be a bit of overengineering considering that we can get like 90% of the benefit with 10% of the effort by following your suggestion :)

I would perhaps not recommend installing svgo globally, since we're requiring a fixed version. However, by installing locally, the command to use it would have to be a bit more verbose — it would need to change from:

-                       svgo ./flags --recursive --config=svgo.config.js

to:

+ node_modules/svgo/bin/svgo ./flags --recursive --config=svgo.config.js

So we might as well declare the dependency in package.json since the file is already there anyway. Then we can use a npm script to run the command for us. It would be as simple as npm install to install the svgo dependency, and npm run svgo to execute it with all the necessary parameters preconfigured. I'll prepare a PR so ou can try this out and let me know how you feel about it :)