search

svg / svgo

⚙️ Node.js tool for optimizing SVG files
https://svgo.dev/
MIT License
20.98k stars 1.39k forks source link

Unique id is broken since v3 #1746

Closed yoriiis closed 1 year ago

yoriiis commented 1 year ago

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 https://github.com/svg/svgo/issues/674#issuecomment-1353701782 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):

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 https://github.com/svg/svgo/issues/674

botmaster commented 1 year ago

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 commented 1 year ago

Try to put prefixIds after cleanupIds

yoriiis commented 1 year ago
yoriiis commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 https://github.com/svg/svgo/pull/1763#issuecomment-1742810271 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_svgcls-1. Now, the exact same SVG ends up with an ID of prefixcls-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 commented 11 months ago

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 commented 11 months ago

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;