feat: make xlink configurable [plugin reusePaths]

[Exotelis]

Updates the reusePaths plugin

Since xlink is deprecated it should be configurable whether it should be used or not.

The implementation is backward compatible, it will not break any existing configuration. However, for version 4.x it might make sense to enable noXlink by default.

• Linter passed
• Tests added and passed
• Types passed

[SethFalco]

Hey hey!

Thanks for investigating this and opening a PR!

I think instead of adding a param to reusePaths, it'd be better to maintain a separate plugin that removes external namespaces to http://www.w3.org/1999/xlink and replaces references to it with the standard href.

This approach was taken in #1535, but it's stalled atm.

If you have time, feel free to take over that PR! Alternatively, when I have time I can rebase it and get it merged for v3.0.4.

TrySound also proposed making that the default behavior, but I'm not sure if that's a good idea myself yet. I'd like for SVGO to preserve (not add) compatibility where possible, so before we enable this by default, we'll need to investigate how widespread support for SVG 2 is.

Closing in favor of: #1535

[Exotelis]

Hey @SethFalco ,

Thank you for taking the time to review my changes.

I have some slight disagreements with a few points you've raised. Firstly, creating a separate plugin to remove existing xlink attributes makes totally sense. However, it's worth noting that plugins are typically executed sequentially. This means that if the removeXlink plugin runs before the reusePaths plugin, it may not find the attribute to remove, as it may not have been added yet.
Additionally, I believe it's not the best software design practice to have one plugin add an attribute only for another plugin to remove it. This approach is error-prone and can lead to potential side effects. In my opinion, it would be more robust to incorporate both change requests. This was also the reason in the first place, why I added my changes directly in the reusePaths plugin.

Please feel free to share your thoughts on these points.

[SethFalco]

However, it's worth noting that plugins are typically executed sequentially.

I appreciate that, so it'd be important to run removeXlink toward the end of the pipeline. We already have a few plugins that depend on running in a certain order.

For example:

• cleanupIds will undo the effects of prefixIds if put later.
• sortAttrs and sortDefsChildren are optimally used at the end.
• removeViewBox and removeDimensions effectively do the opposite of each other.

Something SVGO can do better is present to users that plugins run at a pipeline, and in individual plugins recommend whereabouts the plugin runs best.

Additionally, I believe it's not the best software design practice to have one plugin add an attribute only for another plugin to remove it.

That would apply if we were working in the same function or even file, but not in a plugin-based architecture like SVGO.

This would not be bad design. So long as each plugin individually produces a valid SVG, there is nothing wrong with breaking things down into steps. In fact, that is encouraged by most design patterns.

• Bad software design would be duplicating logic, especially when we consider other plugins may need this option in future too if implemented individually.
• Bad user experience (usability) would be having multiple settings in different places that users would almost certainly want to use together, but feel compelled to configure separately.
• Bad user experience (documentation) would be telling users about multiple ways they can do something, where one is always going to be preferred, so telling them about the other ways is just taking up their time and energy.

I see no harm in plugins taking each other's responsibility internally, but it's not ideal when that gets exposed in the public API, this creates confusion.

Rather than maintain both this logic and documentation multiple times, ask users to spend more time reading the differences, and spend an albeit negligible amount of computation on this condition, users that don't need SVG 1.1 compatibility should just be encouraged to use the removeXlink plugin which will yield better results.

I would rather add a note in the Reuse Paths documentation that if they only need SVG >= 2 compatibility, they're encouraged to add the removeXlink plugin after it, rather than take up more of their time telling them about this param and what the difference is.

In other words, favoring a plugin only approach is more about usability than anything else.

[Exotelis]

Okay I agree we disagree 😀

Personally, I think the DX would be way better if a plugin that creates an output, which might be deprecated, is also responsible for providing a configuration to change its behavior. I don't like the fact reading the docs of some plugin just to find out I need another plugin to "fix" some behavior, which then might again need another one etc. But that's just personal preference and since you are the maintainer your standpoint is absolutely fine 🙂 (just a different approach)

Currently, I'm using a fork with my custom changes as a workaround. I'll try to find some spare time to push the PR you linked forward. I cannot promise to do this within the next week or so :)

And btw thanks for the effort you put into this project