prefixIds breaks references between defs + use

[CH-RhyMoore]

Version: svgo@1.0.4

I have been trying to get SVGO to output unique IDs for elements in a batch of SVGs. I have several SVGs in my iconset that end up visually broken due to colliding IDs.

I thought I finally had a solution when I found the undocumented prefixIds plugin, but when I use prefixIds, it ends up stripping out the <defs> entirely, which just results in SVGs that are still visually broken, only now for a different reason. I am guessing that prefixIds doesn't make the two IDs match up correctly between these two elements, so when removeUselessDefs runs, it gets removed.

This isn't my only problem with the output, although it is the most important as it is blocking my progress. Another noteworthy thing about prefixIds's behavior is I had to (at one point, though perhaps not anymore) enable multipass to get all the optimizations, but with multipass, prefixIds is prefixing twice, which is really an un-optimization.

Related: #595, #674, #766, #834

Example Input SVG:

<?xml version="1.0" encoding="UTF-8"?>
<svg width="24px" height="24px" viewBox="0 0 24 24" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <!-- Generator: Sketch 48.1 (47250) - http://www.bohemiancoding.com/sketch -->
    <title>icon/UI 24px/Star (ratings, reviews)</title>
    <desc>Created with Sketch.</desc>
    <defs>
        <polygon id="path-1" points="10.5 16.5835921 4.3196601 20 5.5 12.763932 0.5 7.63932023 7.4098301 6.58359214 10.5 0 13.5901699 6.58359214 20.5 7.63932023 15.5 12.763932 16.6803399 20"></polygon>
    </defs>
    <g id="Symbols" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
        <g id="icon/UI-24px/Star-(ratings,-reviews)">
            <g id="Fill" transform="translate(1.500000, 2.000000)">
                <mask id="mask-2" fill="white">
                    <use xlink:href="#path-1"></use>
                </mask>
                <use id="Shape" fill="#000000" fill-rule="nonzero" xlink:href="#path-1"></use>
                <g id="color/primary/black-100" transform="translate(-1.500000, -2.000000)" fill="#3E3939">
                    <rect id="black-100" x="0" y="0" width="24" height="24"></rect>
                </g>
            </g>
        </g>
    </g>
</svg>

Example Output SVG without prefixIds:

<svg xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" pointer-events="none" viewBox="0 0 24 24">
  <defs>
    <path id="a" d="M10.5 16.584L4.32 20l1.18-7.236-5-5.125 6.91-1.055L10.5 0l3.09 6.584 6.91 1.055-5 5.125L16.68 20z"/>
  </defs>
  <g fill="currentColor" fill-rule="evenodd" transform="translate(1.5 2)">
    <use fill-rule="nonzero" xlink:href="#a"/>
    <path d="M-1.5-2h24v24h-24z"/>
  </g>
</svg>

Example Output SVG with prefixIds and multipass:

<svg xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" pointer-events="none" viewBox="0 0 24 24">
  <g fill="currentColor" fill-rule="evenodd" transform="translate(1.5 2)">
    <use fill-rule="nonzero" xlink:href="#svg2017__svg2012__path-1"/>
    <path d="M-1.5-2h24v24h-24z"/>
  </g>
</svg>

Example Output SVG with prefixIds and single pass:

<svg xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24" pointer-events="none">
  <g fill="currentColor" fill-rule="evenodd" transform="translate(1.5 2)">
    <use fill-rule="nonzero" xlink:href="#svg1318__path-1"/>
    <path d="M-1.5-2h24v24h-24z"/>
  </g>
</svg>

Configuration:

let count = 0;

module.exports = {
  full: true,
  multipass: true,
  precision: 3,
  // order of plugins is important to correct functionality
  plugins: [
    { removeDoctype: true },
    { removeXMLProcInst: true },
    { removeComments: true },
    { removeMetadata: true },
    { removeXMLNS: true },
    { removeEditorsNSData: true },
    { cleanupAttrs: true },
    { inlineStyles: true },
    { minifyStyles: true },
    { convertStyleToAttrs: true },
    {
      prefixIds: {
        prefix: function () {
          const prefix = `svg${count}`;
          count++;
          return prefix;
        }
      }
    },
    { cleanupIds: true },
    { removeRasterImages: true },
    { removeUselessDefs: true },
    { cleanupNumericValues: true },
    { cleanupListOfValues: true },
    { convertColors: { currentColor: true } },
    { removeUnknownsAndDefaults: true },
    { removeNonInheritableGroupAttrs: true },
    { removeUselessStrokeAndFill: true },
    { removeViewBox: false },
    { cleanupEnableBackground: true },
    { removeHiddenElems: true },
    { removeEmptyText: true },
    { convertShapeToPath: true },
    { moveElemsAttrsToGroup: true },
    { moveGroupAttrsToElems: true },
    { collapseGroups: true },
    { convertPathData: true },
    { convertTransform: true },
    { removeEmptyAttrs: true },
    { removeEmptyContainers: true },
    { mergePaths: true },
    { removeUnusedNS: true },
    { sortAttrs: true },
    { removeTitle: true },
    { removeDesc: true },
    { removeDimensions: false },
    { removeAttrs: false },
    { removeElementsByAttr: false },
    { addClassesToSVGElement: false },
    { removeStyleElement: true },
    { removeScriptElement: true },
    {
      addAttributesToSVGElement: {
        attributes: [ { 'pointer-events': 'none' } ]
      }
    }
  ],
  js2svg: {
    pretty: true,
    indent: '  '
  }
};

(This part is mostly rhetorical . Specific examples of some of the other things I didn't expect: why is the def/use kept, when the polygon could easily be substituted as it's only used once? Why is the translate transform not being flattened? I'd've sworn I remembered SVGO having that feature, although I see an open issues #594, #624.)

[strarsis]

@CH-RhyMoore: Apparently the prefixIds plugin is run before another
plugin further changes the SVG (like changing the <def>/<use>).
So the prefixIds plugin either has to be moved further down to run after the other optimizations.

[CH-RhyMoore]

@strarsis Given that the order of plugins in the configuration matters, but that there is no explicit documentation of the correct order, I have been patterning my order off of files in the repo, like this:

https://github.com/svg/svgo/blob/master/.svgo.yml#L30-L32

Perhaps the order in the README is preferable? If so, where should prefixIds be in that list? I'm configuring all the plugins at once, as you can see, so any recommendation given to me would be generically useful, not unique to me. I am not keen on moving it down one by one until I find the "right" place.

[CH-RhyMoore]

@strarsis To test, I moved it down to the bottom and while the defs are not removed, the IDs still don't match.

<svg xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24" pointer-events="none">
  <defs>
    <path id="svg400__a" d="M10.5 16.584L4.32 20l1.18-7.236-5-5.125 6.91-1.055L10.5 0l3.09 6.584 6.91 1.055-5 5.125L16.68 20z"/>
  </defs>
  <g fill="currentColor" fill-rule="evenodd" transform="translate(1.5 2)">
    <use fill-rule="nonzero" xlink:href="#svg402__a"/>
    <path d="M-1.5-2h24v24h-24z"/>
  </g>
</svg>

This results in the following visual:

I observed that the prefixIds prefix function runs per element, which to me seems like it would of course generate a different prefix for IDs that are supposed to match on different elements within a file.

Note for posterity: The ID issues alone would actually result in this example SVG appearing blank. The black box would still occlude the expected shape even with the ID issue fixed--it comes from the original export and has to be fixed in Sketch. I was having two issues at the same time when I posted.

[CH-RhyMoore]

There's another glitch in the original export that is coming from a technique used in Sketch, so while I still have visually broken SVGs, it's no longer due to the ID issues.

I didn't really need to move prefixIds. I just needed the prefix to match throughout a file. However, the prefix function gets called once for every element in a file and I needed a way to return the same prefix for every element in a file while being unique across different files.

Unfortunately, what's passed to the prefix function by default is information about the current element; there's no other context available. But there's an optional second argument info to svg.optimize. Whatever you pass in as info is also passed as the second argument of the prefix function. So I passed the file path, then hashed it to get something:

• Short,
• With characters valid for IDs (I have looong filenames with spaces, parentheses, and periods--and note because the hash yields a number, I had to prefix my prefix--IDs can't start with numbers),
• The same within a file,
• And different in different files.

function optimize ({ contents, path }) {
  return svgo.optimize(contents, path);
}

const hash = require('string-hash');
...
prefixIds: {
  prefix: function (element, filepath) {
    return `svg${hash(filepath)}`;
  }
}

<svg xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24" pointer-events="none">
  <defs>
    <path id="svg461883165__a" d="M10.5 16.584L4.32 20l1.18-7.236-5-5.125 6.91-1.055L10.5 0l3.09 6.584 6.91 1.055-5 5.125L16.68 20z"/>
  </defs>
  <g fill="currentColor" fill-rule="evenodd" transform="translate(1.5 2)">
    <use fill-rule="nonzero" xlink:href="#svg461883165__a"/>
    <path d="M-1.5-2h24v24h-24z"/>
  </g>
</svg>

This keeps the references between use and thepath intact, so the used defs don't get removed, even if prefixIds is in the original order.

[strarsis]

@CH-RhyMoore: Would you recommend extra parameters to be passed to
the prefixID callback function so this is easier in the future?

[CH-RhyMoore]

@strarsis I think so, yes. If it's possible. The reason I think it'd be a good idea is that, from the issues, I got the impression one of the use cases you wrote it for is being able to get unique IDs that don't collide between files. But the first few obvious choices for using it (some of which in my case were inspired by tests/issues discussion/the PR) can lead a user in the wrong direction. For that use case, a user doesn't want to make IDs that are supposed to be the same in the same file different from each other, but it's not immediately obvious that that needs extra attention when you are customizing it. Once you follow all the bread crumbs, it's not hard code to write, but it did take a while to find some of the crumbs.

I assume that challenge of correlating IDs between elements comes from how the plugins run in general, not from this specific plugin.

[CH-RhyMoore]

@strarsis One specific idea that could help is passing the default prefix in. You derive that from the filename, so whether someone wants to customize on top of the filename, do something like I did, or needs to do something really unusual, it'll provide the context essential to keeping references intact.

[strarsis]

@CH-RhyMoore:
Indeed, the prefixIds plugin should offer an additional option for appending or prepending to the filename as prefix instead just using it as the prefix (which would be the same for different input SVG files).
After your explanation I think it probably makes more sense to actually rename the prefix option to hash.

Currently the prefix callback is called with parameters node and extra.
If no prefix callback was passed, the filename is extracted from the path to input SVG file.
This path is available in extra.path:

var filename = path.basename(extra.path);
prefix = filename;

For prefixing the filename by passing a custom prefix function you can use:

prefixIds: {
  prefix: function (element, extra) {
    var filename = path.basename(extra.path);
    ...
    return filename + prefix;
  }
}

Concerning an improved option feature for configuring hashes used by the prefixIds plugin:
What kind of structure should be used? What about something similar to what gulp-rename is using?

{
  ...
  prefixIds: {
    hash: {
      prefix: "some prefix-",
      suffix: "-some suffix"
    }
  },
  ...
}

And for the current behaviour (just always using the same prefix):

{
  ...
  prefixIds: {
    hash: "use always this hash"
  },
  ...
}

Preserving the ability to pass a callback function for generating the hash:

{
  ...
  prefixIds: {
    hash: function(node, extra){...}
  },
  ...
}

Then there is the delim option. Should it be skipped when a hash is passed?
Should it be put between the suffix/postfix? In the end the user should always be able to completely modify the behaviour by passing a function for the hash.

[vzaidman]

A first good step is proposed in my PR #907 and it lets you use it like this:

plugins: [
          {convertPathData: false},
          {prefixIds: {
            prefix: node => {
              const elementName = node && node.elem
              if(!elementName || elementName === 'svg' || elementName === 'use'){
                return false
              }

              return 'some-prefix'
            }
          }}
        ]

or like this

plugins: [
          {convertPathData: false},
          {prefixIds: {prefix: false}}
        ]

[strarsis]

@vzaidman: And for using the input file name for the prefix?

[vzaidman]

for now it works like what you wrote isn't it?

[vzaidman]

maybe we should add a plugin by the name "transformId" instead of "prefixId" (deprecate it) and let the user transform the Id to whatever he likes:

plugins: [
          {convertPathData: false},
          {transformIds: (id, element, extra) => {
             const filename = path.basename(extra.path)
             const hash = `#${uuid()}`
             return `something_${Number(id) + 2}_${filename}_${hash}`
          }
        ]

[SilverFox70]

If anyone is looking to generate unique ID while using svg-react-loader, I have a fix here : https://github.com/SilverFox70/svg-react-loader