refactor: remove is-svg

[ludofischer]

Hopefully this will stop further alarmed comments.

[alexander-akait]

Need fix CI

[ludofischer]

Those look like legit test failures. I'l need to investigate what is happening.

[ludofischer]

Cannot update past is-svg 4.2.2, which luckily contains the CVE fix.

Interestingly there's a regression in is-svg 4.3.x, it fails to reject strings that contain random text after the svg.

See the is-svg test: https://github.com/sindresorhus/is-svg/blob/034967d4053dd6ca907a6bfeb047cd61b4c12f0a/test.js#L39
and the corresponding issue in the library that is-svg uses: NaturalIntelligence/fast-xml-parser#327

[alexander-akait]

We should wait fix in this case

[ludofischer]

We could maybe skip the isSvg step completely. Since is-svg was never 100% accurate, some non-SVGs things are caught at the isSvg step and some in the svgo.optimize() step. I wonder if we could just try svgo.optimize() and catch the error. That way we would also stop depending on two different XML parsers (the one that comes from is-svg and the one from svgo). The problem is that we would feed a lot of the non-svg things inside url() to SVGO, but in fact both is-svg and SVGO start by trying to parse the input as XML, they just offer a different interface for reporting errors.

[alexander-akait]

Sounds good

[ludofischer]

I have changed these tests because I 've realized there is no meaningful difference between these cases. They're all invalid SVGs. They were handled separately because some failed at the isSvg stage and some at the svgo stage.

[ludofischer]

I could not find a better way to detect the cause of the error. Maybe @TrySound has got a better suggestion? Anyway I wonder whether it makes sense to throw an error just because we cannot process sommething, instead of just leaving it alone. See this comment #910 (comment)

[alexander-akait]

I think we don't need throw the error in this cases (and all cases, for example postcss-calc, if we can't parse it just log warning, so developers can report about it and we still continue work), let's use console.warn and continue work

[TrySound]

Doesn't postcss have warn method?

[alexander-akait]

oh, yep, forgot about it

[TrySound]

I don't get why do we even need to check something? data:image/svg+xml implies we deal with svg. So we run svgo with it and if this svg is invalid throw error. Am I missing something?

[ludofischer]

I don't get why do we even need to check something? data:image/svg+xml implies we deal with svg. So we run svgo with it and if this svg is invalid throw error. Am I missing something?

I 99% agree with you. Except in the context of cssnano I would not throw on invalid SVG , as it might be preferable to skip minification of that particular section rather than crashing completely.
After all, cssnano is not a linter.

[alexander-akait]

So let's avoid using throw new Error and just use postcss warn method, in future we should avoid using throw new Error everything