Skip to content
New issue

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

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

Already on GitHub? # to your account

Allow --input to be a single file when --output is a folder #1675

Closed

Conversation

burntcustard
Copy link

@burntcustard burntcustard commented May 13, 2022

The README currently has example CLI usage, where you can either:
a) process one or more single input files into the same number of output files:
svgo one.svg two.svg -o one.min.svg two.min.svg
or
b) process a directory of files, into an output directory:
svgo -f ./path/to/folder/with/svg/files -o ./path/to/folder/with/svg/output

I had hoped that I'd be able to do an in-between, with a single file as the input and a directory as the --output:
svgo one.svg -o ./path/to/folder/with/svg/output

That would let me use chokidar in my project's package.json, to watch any SVGs that are updated, and re-run SVGO on them:
"svg:watch": "chokidar src/assets/svg/*.svg -c 'svgo --input {path} --output dist/assets/svg'"

Unfortunately, I ran into an issue where if any layer of the output directory did not exist before SVGO ran, it would have unexpected results, like Error: ENOENT: no such file or directory, or creating a weird assets/svg file, that should have been assets/svg/mySvg.svg.

My fix replaces the check to see if a single --output string matches a pre-existing directory, with a check that it's simply a directory string (rather than a filename string with a file extension). That way, if you intend to put files into an output directory, but the directory doesn't yet exist, the surrounding code in coa.js successfully creates both the intended directory and output file inside it.

If this PR is accepted, it might be helpful to put a relevant single-input folder-output example in the CLI usage section of the README, to show that this can now be done?

@burntcustard
Copy link
Author

Apologies for not catching the output-as-stream test failure. I've put in an extra check to revert to previous behaviour when output[0] is -, and it passes locally.

I'm not really a fan of the non-strict comparisons being used in the line I'm changing, but it is at least consistent with the rest of the code, and doesn't negatively effect anything AFAIK.

@@ -215,7 +215,7 @@ async function action(args, opts, command) {
// --output
if (output) {
if (input.length && input[0] != '-') {
if (output.length == 1 && checkIsDir(output[0])) {
if (output.length == 1 && output[0] != '-' && !path.extname(output[0])) {
Copy link
Member

@SethFalco SethFalco Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, it took so long for someone to review your PR! We're working on the backlog now. ^-^'


I disagree with this approach because it leads to unexpected behavior in other cases, for example:

svgo original.svg -o minified

I would expect this to create a file, not a directory, called minified.

Imo, a better solution would be to update checkIsDir to return true when the path ends with a trailing path.sep, which implies it's a directory, so there's less ambiguity to what the user wants.

/**
 * Synchronously check if path is a directory. Tolerant to errors like ENOENT.
 *
 * @param {string} filePath
 */
export function checkIsDir(filePath) {
  try {
    return fs.lstatSync(filePath).isDirectory();
  } catch (e) {
    return filePath.endsWith(path.sep);
  }
}
# output: minified
svgo original.svg -o minified

# output: minified.svg
svgo original.svg -o minified.svg

# output: dist/minified.svg
svgo original.svg -o dist/minified.svg

# output: dist/original.svg
svgo original.svg -o dist/

Now, when the output path ends with an explicit / (or \ on Windows) we know you mean a directory. Backward compatibility is less of a concern because SVGO previously crashed in this scenario anyway. (When the path ends with a trailing slash.)

Does this sound like a suitable solution to you?

@SethFalco
Copy link
Member

This has been handled with the approach proposed in my comment. It'll be released in the next version of SVGO. 👍🏽

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

Successfully merging this pull request may close these issues.

2 participants