Disable cleanupIds by default

[chancehudson]

Is your feature request related to a problem? Please describe.
The cleanupIds option does not check for id uniqueness globally. This causes a page with multiple SVGs to have id collisions. This is causing problems in downstream projects, particularly docusaurus.

Describe the solution you'd like
cleanupIds is disabled by default.

[TrySound]

SVGO by default optimizes svg image to not break when used as is. Any optimization can break embedded svg. Scripts or styles can access svg structure or rely on specific values. It's up to specific usage what to disable. Also collisions are possible with existing ids. You can add prefixIds plugin if not embed same svgs twice.

[chancehudson]

Yes that's true, I think my problem with this is if I used SVGO and do nothing but add 2 svgs to a webpage, they will be broken by default. This seems like a bad dev experience.

True collisions can happen by default as well, but at least in that case it's easier to debug/fix. It's difficult to debug the problem in upstream projects because they're sometimes separated by many layers of dependencies.

I guess the real solution is minification that tracks the global context to prevent collisions, but I imagine that's difficult/impossible in webpack otherwise you would have done it.

Anyway, it's up to you, the problem is easily solved with a config. Also, please don't take my criticism as a lack of appreciation, great work here.

[SethFalco]

I'll close this in favor of #1746 which dives into more detail.

I'm not sure what's the best action to take here yet, especially as this is causing issues downstream, like with Docusaurus. 🤔

I think cleanupIds is a sensible default. However, disabling this isn't a viable solution because IDs can always conflict whether they pass through SVGO or not. Something will need to be done to actively prevent it in the context of Docusaurus.

Something that could alleviate the issue is to introduce a setting like globallyUniqueIds. So when SVGO is passed a directory or multiple SVGs in the same process, we persist a set of used IDs and classes between the SVGs, to avoid using them again.

I will investigate if there's anything SVGO can do to make it easier to manage. An immediate solution is probably to use prefixIds, though some users have reported problems with that which I need to investigate.

[hashimaziz1]

@SethFalco Just wondering, is there a reason you closed this issue? Asking because I noticed in my tests that SVGO no longer seemed to be minifying IDs - maybe because you ended up disabling cleanupIds in the end?

[SethFalco]

I explain why in my comment above!

I'll close this in favor of #1746 which dives into more detail.

This issue should not be resolved yet, there's just another issue that covers the same bug, but is more explicit, that's all.

PS: I am working on some documentation which includes the workarounds for this issue:
https://svgo.dev/docs/plugins/cleanupIds/

[hashimaziz1]

I explain why in my comment above!

I'll close this in favor of #1746 which dives into more detail.

This issue should not be resolved yet, there's just another issue that covers the same bug, but is more explicit, that's all.

PS: I am working on some documentation which includes the workarounds for this issue: https://svgojs.org/docs/plugins/cleanup-ids/

I see, I suppose I was expecting that there would be some changes that resulted in cleanupIds not working for me. In that case, do you have any idea why it wouldn't be minifying for me on a test file with the following config:

module.exports = {
  multipass: true,
  plugins: [
    {
      // Include and override the built-in plugins
      name: "preset-default",
      params: {
        overrides: {
          removeViewBox: false,
          removeTitle: false,
          removeDesc: false,
        },
      },
    },
    {
      name: "removeUnknownsAndDefaults",
      params: {
        keepRoleAttr: true,
      },
    },

    "cleanupIds",
    "prefixIds",
  ],
};

I've come across prefixIds before and it is working fine, so currently all I'm getting is file_name__original_id when I'm expecting file_name__a.

I have also come across that documentation site, great work and I can't wait to see it and the positive impact it'll have on this tool when it's publicly released.

[SethFalco]

Hmm, immediately I can't tell. 🤔
Would you be willing to share the file you're testing with, or a minimal-reproducible-example?

If there's a problem with sharing the file publicly, I'd be happy for you to email it to me instead. You can find my email on my GitHub profile.

[hashimaziz1]

Hmm, immediately I can't tell. 🤔 Would you be willing to share the file you're testing with, or a minimal-reproducible-example?

If there's a problem with sharing the file publicly, I'd be happy for you to email it to me instead. You can find my email on my GitHub profile.

Sent, you may need to check your spam.

[SethFalco]

Sent, you may need to check your spam.

Oh, I already received and responded to your email. ^-^'
It might be that you haven't received mine. 🤔

If not, would you mind adding me as a trusted/safe sender and I can try again perhaps? Google seems to be fine, but everyone else is mad at me for having .fun in my email address. ^-^'

[hashimaziz1]

Sent, you may need to check your spam.

Oh, I already received and responded to your email. ^-^' It might be that you haven't received mine. 🤔

If not, would you mind adding me as a trusted/safe sender and I can try again perhaps? Google seems to be fine, but everyone else is mad at me for having .fun in my email address. ^-^'

Yeah, it was me that needed to check my spam after all. You'd think a response from someone I emailed first wouldn't be marked as spam, but that's Outlook for you. Now replied.