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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
300fa13
Check if --output has no extname instead of is a dir that exists
burntcustard May 13, 2022
975083b
Fix for test not working (check if not trying to output stream)
burntcustard May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/svgo/coa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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?

var dir = output[0];
for (var i = 0; i < input.length; i++) {
output[i] = checkIsDir(input[i])
Expand Down