svgo icon indicating copy to clipboard operation
svgo copied to clipboard

Run plugins in a predefined order

Open noahbald opened this issue 1 year ago • 10 comments

Is your feature request related to a problem? Please describe. Certain plugins perform better based on a prior plugin. Consider the following convertPathData, then convertShapeToPath

<!-- before -->
    <rect id="useless" width="0" height="0" fill="#ff0000"/>
<!-- `convertPathData`, then `convertShapeToPath` -->
    <path id="useless" fill="#ff0000" d="M0 0H0V0H0z"/>
<!-- `convertShapeToPath`, then `convertPathData` -->
    <path id="useless" fill="#ff0000" d="M0 0"/>

Describe the solution you'd like Right now the order plugins are run in is in the order provided in the config, however this can be suboptimal. For example, it makes sense to run convertShapeToPath before convertPathData.

Describe alternatives you've considered N/A

Additional context N/A

noahbald avatar Nov 07 '24 20:11 noahbald

Right now the order plugins are run in is in the order provided in the config, however this can be suboptimal.

Is updating your config an option here? You should be able to just move around the plugins in your config to match your preferred order.

KTibow avatar Nov 08 '24 15:11 KTibow

For sure, but given that there exists an ideal order, I think it makes sense to use that by default rather than leave it to the user?

noahbald avatar Nov 08 '24 22:11 noahbald

So you think the order of plugins in preset-default should be changed?

KTibow avatar Nov 08 '24 23:11 KTibow

Not just the preset, but any given set of plugins should run in an ideal order.

if you check out the example I gave, it demonstrates that some plugins should always run before others to produce an optimal output

noahbald avatar Nov 09 '24 01:11 noahbald

That's a breaking change, and IMO a breaking change that doesn't make any sense

KTibow avatar Nov 09 '24 01:11 KTibow

Not sure what’s not making sense? And besides, being a breaking change is fine if it’s part of a major release?

If it would be helpful for me to look into it more than just the one example I’ve found, I can certainly do so.

If it’s something that’s not worth it for the project, all good too, just a suggestion :)

noahbald avatar Nov 09 '24 03:11 noahbald

Well SVGO is a composition of plugins, and right now, they act like a pipeline. This makes it easy to change the order of plugins or add custom plugins.

Even if you changed it to be more like toggles, that would just hide the underlying pipeline.

I feel like the problems here would be better addressed by updating whatever pipeline is being ran rather than changing how plugins are specified, but that's just the opinion of this random guy who watches this repo.

KTibow avatar Nov 09 '24 05:11 KTibow

Hey hey! Thanks for opening the issue!

This issue as proposed initially isn't going to happen. SVGO runs as a pipeline by design, and if someone wants to configure the pipeline order themselves, it's on the user to put plugins in the optimal order for their use-case. (As you have discovered yourself, our order isn't always perfect either.)

However, I'd be happy to reconsider the order of the plugins in preset-default as we're planning a major release soon anyway, or to add hints in the documentation where certain configurations may be favorable, regardless of the upcoming major release.

I'll explore the two plugins referenced in this issue soon, and see if I agree that that should run the other way around. If so, we could do this as part of v4.0.0. Immediately, this sounds very reasonable, though, but I just want to play it safe and investigate anyway.

SethFalco avatar May 11 '25 20:05 SethFalco

This issue as proposed initially isn't going to happen. [...] I'll explore the two plugins referenced in this issue soon, and see if I agree that that should run the other way around. If so, we could do this as part of v4.0.0.

Looking at https://github.com/svg/svgo/blob/bd25d8ab05479278c18c6e390eaad3829c0f06ca/plugins/preset-default.js#L60-L65 it seems that these plugins were already listed in the ideal order long before this issue was created.

Given that 4.0.0 has been released, should the issue now get closed as not planned?

mootari avatar Jun 22 '25 17:06 mootari

Oh shoot! I actually forgot to come back to this one! My fault for not assigning it to myself. ^-^'

Understood, so the issue existed only in your config, but not in our presets. I misunderstood the issue, and thought the comment was regarding preset-default too. Thank you for checking preset-default on your end, though.

In the short-term, we definitely won't move away from the pipeline approach, and I don't want us to touch the pipeline the user defined. At least, not automatically.

But you can leave this open for now, and we'll think later of if there's something we want to do about it.

  • Recommendations in the docs. (We do this already for some plugins.)
  • A warning at runtime. (Not into this.)
  • A separate (opt-in) arg that tells SVGO it can manipulate the plugin order.
  • A separate arg that lints the svgo.config.js instead of optimizing. (Malformed config, plugin order recommendations, or whatever else.)
  • etc. We'll see! But the motivation for the issue is very valid! I just didn't want to do the proposed solution in v4.

SethFalco avatar Jun 22 '25 18:06 SethFalco