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.

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

Unique id is broken since v3 #1746

Closed
yoriiis opened this issue Jan 27, 2023 · 8 comments · Fixed by #1814
Closed

Unique id is broken since v3 #1746

yoriiis opened this issue Jan 27, 2023 · 8 comments · Fixed by #1814
Labels

Comments

@yoriiis
Copy link

yoriiis commented Jan 27, 2023

Describe the bug

I need unique ids when multiple SVGs have <defs>. With v2.8 I was using the following code and it works well.

{
    name: 'preset-default',
    params: {
        overrides: {
            cleanupIDs: {
                prefix: {
                    toString() {
                        return crypto.randomBytes(20).toString('hex').slice(0, 4)
                    }
                }
            }
        }
    }
}

But in v3 the prefix parameter of cleanupIDs is removed replacing prefixIds. I can't get the same rendering for the IDs.
I've tested this code but it breaks the <defs>.

{
    name: 'prefixIds',
    params: {
        delim: '',
        prefix: function (node, info) {
            return crypto.randomBytes(20).toString('hex').slice(0, 4)
        }
    }
}

In input there are 2 svgs

gradient.svg

<svgxmlns="http://www.w3.org/2000/svg"><defs><linearGradient id="MyGradient"><stop offset="5%" stop-color="green"/><stop offset="95%" stop-color="gold"/></linearGradient></defs><rect fill="url(#MyGradient)" width="100%" height="100%"/></svg>

delete-alert.svg

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 19"><defs><linearGradient id="a" x1="0%" x2="100%" y1="9.5%" y2="90.5%"><stop offset="0%" stop-color="#FF0053"/><stop offset="100%" stop-color="#EE5B35"/></linearGradient></defs><g fill="none"><path fill="url(#a)" d="M15 8a5 5 0 110 10 5 5 0 010-10zm-9.81 7L9 16.36c-.48 1.27-1.72 1.9-2.77 1.54-1.04-.36-1.52-1.63-1.04-2.9zM11.2.84c.2-.68.92-.98 1.63-.78.71.2 1.02.88.82 1.56l-.41.98A5.87 5.87 0 0116 6.4h-.2c-3.98 0-7.14 3.03-7.14 6.84 0 .59 0 1.17.2 1.76L0 11.88l.3-.79 2.25-.97 1.73-4.69a6.1 6.1 0 016.62-3.71z" transform="translate(0 .5)"/><path fill="#FFF" d="M12 13h6v1h-6z"/></g></svg>

In output, with the prefixIds plugin, it produced

<svg aria-hidden="true" style="position: absolute; width: 0; height: 0; overflow: hidden;"><defs/><symbol id="gradient"><rect width="100%" height="100%" fill="url(#69452d92ac6ca)"/></symbol><symbol id="delete-alert" viewBox="0 0 20 19"><g fill="none"><path fill="url(#5fcf12e19c5fa)" d="M15 8a5 5 0 1 1 0 10 5 5 0 0 1 0-10zm-9.81 7L9 16.36c-.48 1.27-1.72 1.9-2.77 1.54-1.04-.36-1.52-1.63-1.04-2.9zM11.2.84c.2-.68.92-.98 1.63-.78.71.2 1.02.88.82 1.56l-.41.98A5.87 5.87 0 0 1 16 6.4h-.2c-3.98 0-7.14 3.03-7.14 6.84 0 .59 0 1.17.2 1.76L0 11.88l.3-.79 2.25-.97 1.73-4.69a6.1 6.1 0 0 1 6.62-3.71z" transform="translate(0 .5)"/><path fill="#FFF" d="M12 13h6v1h-6z"/></g></symbol></svg>

I've also tested the solution from #674 (comment) but there is always ids conflics. It produces the following output:

<svg aria-hidden="true" style="position: absolute; width: 0; height: 0; overflow: hidden;"><defs><linearGradient id="1qorephmpqg__a"><stop offset="5%" stop-color="green"/><stop offset="95%" stop-color="gold"/></linearGradient><linearGradient id="1qorephmpqg__a" x1="0%" x2="100%" y1="9.5%" y2="90.5%"><stop offset="0%" stop-color="#FF0053"/><stop offset="100%" stop-color="#EE5B35"/></linearGradient></defs><symbol id="gradient"><rect width="100%" height="100%" fill="url(#1qorephmpqg__a)"/></symbol><symbol id="delete-alert" viewBox="0 0 20 19"><g fill="none"><path fill="url(#1qorephmpqg__a)" d="M15 8a5 5 0 1 1 0 10 5 5 0 0 1 0-10zm-9.81 7L9 16.36c-.48 1.27-1.72 1.9-2.77 1.54-1.04-.36-1.52-1.63-1.04-2.9zM11.2.84c.2-.68.92-.98 1.63-.78.71.2 1.02.88.82 1.56l-.41.98A5.87 5.87 0 0 1 16 6.4h-.2c-3.98 0-7.14 3.03-7.14 6.84 0 .59 0 1.17.2 1.76L0 11.88l.3-.79 2.25-.97 1.73-4.69a6.1 6.1 0 0 1 6.62-3.71z" transform="translate(0 .5)"/><path fill="#FFF" d="M12 13h6v1h-6z"/></g></symbol></svg>

To Reproduce

I don't think it's necessary, tell me if necessary

Expected behavior

Unique ids should be possible as in v.2.8.

Screenshots

Desktop (please complete the following information):

  • SVGO : 3.0.2
  • NodeJs : 14.21.2
  • OS: macOS 12.6

Additional context

The package is used inside the svg-chunk-webpack-plugin. All SVGO options are transmitted from plugin users.
The question was posted on the SVGO Discord channel, without response (See question on SVGO Discord).

This issue is linked to #674

@yoriiis yoriiis added the bug label Jan 27, 2023
@botmaster
Copy link

Hello
Exactly the same issue.
It fail when the returned value is computed 😕

Work:

      svgoConfig: {
        plugins: [
          {
            name: "prefixIds",
            params: {
              prefixIds: true,
              prefixClassNames: false,
              prefix(node, info) {
                return `toto`;
              },
              delim: "-",
            },
          },
          {
            name: "cleanupIds",
            params: {
              minify: false,
            },
          },
        ],
      }

Doesn't work:

      svgoConfig: {
        plugins: [
          {
            name: "prefixIds",
            params: {
              prefixIds: true,
              prefixClassNames: false,
              prefix(node, info) {
                return `toto${Math.random().toString()}`;
              },
              delim: "-",
            },
          },
          {
            name: "cleanupIds",
            params: {
              minify: false,
            },
          },
        ],
      }

@TrySound
Copy link
Member

Try to put prefixIds after cleanupIds

@yoriiis
Copy link
Author

yoriiis commented Feb 16, 2023

  • With prefixIds after cleanupIds there are ids conflics.
  • With prefixIds before cleanupIds the prefix is not applied.

@yoriiis
Copy link
Author

yoriiis commented Mar 2, 2023

@TrySound can you help us on this please?
The decision to remove the cleanupIds prefix plugin was made in the v3 release so I imagine you have an alternative solution? Thanks

@mryechkin
Copy link

Running into this issue as well. Without any changes to the source SVG, previously I would get an ID like youtube_svg__cls-1. Now, the exact same SVG ends up with an ID of prefix__cls-1.

This is my config:

module.exports = {
  js2svg: {
    indent: 2,
    pretty: true,
  },
  plugins: [
    {
      name: 'preset-default',
      params: {
        overrides: {
          removeViewBox: false,
        },
      },
    },
    'convertStyleToAttrs',
    'prefixIds',
    'removeDimensions',
  ],
};

Any suggestions on how to fix this please? @TrySound

@SethFalco
Copy link
Member

SethFalco commented Oct 23, 2023

@botmaster

Try to put prefixIds after cleanupIds

TrySound is correct, prefixIds must be after cleanupIds. Doing it the other way around would make cleanupIds undo prefixIds.


@yoriiis

so I imagine you have an alternative solution? Thanks

The alternate solution was to use prefixIds, however it had a bug. Turns out the same bug I noticed and documented in #1763 (comment) was the bug that was causing you grief here as well.

I've opened the following PR:

With this, I've tested both of your examples using the following configuration, and can confirm they no longer break.

const crypto = require('crypto');

module.exports = {
  plugins: [
    'preset-default',
    {
      name: 'prefixIds',
      params: {
        delim: '',
        prefix: () => crypto.randomBytes(20).toString('hex').slice(0, 4)
      }
    }
  ],
};

Sorry for the headache, and thanks for being patient with this. The fix will be released in v3.0.3.


@mryechkin

previously I would get an ID like youtube_svg__cls-1. Now, the exact same SVG ends up with an ID of prefix__cls-1.

This CLI works fine for me, but if you're using the JS API, it may be because you didn't provide the path option to #optimize?

More info: https://github.com/svg/svgo#optimize

By default, prefixIds only uses the file name if it's known. If not, then it uses prefix__ instead.

@karlhorky
Copy link

karlhorky commented Nov 9, 2023

Thanks so much @SethFalco 🙌 Can confirm this works in our project 👍


Here's a modern ES Modules + JSDoc version of the randomBytes config:

import { randomBytes } from 'node:crypto';

/** @type {import('svgo').Config} */
const svgoConfig = {
  plugins: [
    'preset-default',

    // Avoid collisions with ids in other SVGs,
    // which was causing incorrect gradient directions
    // https://github.com/svg/svgo/issues/1746#issuecomment-1803595909
    //
    // Previously, this was a problem where unique ids
    // could not be generated since svgo@3
    // https://github.com/svg/svgo/issues/674
    // https://github.com/svg/svgo/issues/1746
    {
      name: 'prefixIds',
      params: {
        delim: '',
        prefix: () => randomBytes(20).toString('hex').slice(0, 4),
      },
    },
  ],
};

export default svgoConfig;

@karlhorky
Copy link

karlhorky commented Nov 9, 2023

A simpler alternative which does not rely on any imports is a simple counter, can also confirm this works in our project:

let svgoPrefixIdsCount = 0;

/** @type {import('svgo').Config} */
const svgoConfig = {
  plugins: [
    'preset-default',

    // Avoid collisions with ids in other SVGs,
    // which was causing incorrect gradient directions
    // https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
    //
    // Previously, this was a problem where unique ids
    // could not be generated since svgo@3
    // https://github.com/svg/svgo/issues/674
    // https://github.com/svg/svgo/issues/1746
    {
      name: 'prefixIds',
      params: {
        delim: '',
        prefix: () => svgoPrefixIdsCount++,
      },
    },
  ],
};

export default svgoConfig;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants