Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More explicit svgo version #99

Open
waldyrious opened this issue Jun 9, 2023 · 4 comments · May be fixed by #100
Open

More explicit svgo version #99

waldyrious opened this issue Jun 9, 2023 · 4 comments · May be fixed by #100

Comments

@waldyrious
Copy link
Collaborator

Moving a discussion started in c9eed38 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. #93.

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

@HatScripts
Copy link
Owner

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

@waldyrious
Copy link
Collaborator Author

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
Copy link
Owner

HatScripts commented Jun 9, 2023

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
Copy link
Collaborator Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants