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

Enabling both cleanupIDs and prefixIds does not work as expected #1406

Closed
dannobytes opened this issue Mar 5, 2021 · 2 comments
Closed

Enabling both cleanupIDs and prefixIds does not work as expected #1406

dannobytes opened this issue Mar 5, 2021 · 2 comments
Labels

Comments

@dannobytes
Copy link

Describe the bug
When attempting to optimize with both cleanupIDs and prefixIds plugins enabled, I expect the IDs to be minified AND prefixed with the SVG filename. However, it instead only prefixes the IDs with the filename if cleanupIDs.minify is disabled.

There are valid reasons why minifying the ID and then prefixing it with the filename are necessary, mainly when inlining multiple SVGs on the same page. Without unique prefixes, duplicate IDs will conflict with one another.

To Reproduce
Steps to reproduce the behavior:

  1. Enable both plugins
// svgo.config.js
const { extendDefaultPlugins } = require('svgo')

module.exports = {
  multipass: true,
  plugins: extendDefaultPlugins([
    'cleanupIDs',
    'prefixIds'
  ])
}
  1. Run the optimizer on an SVG that contains IDs. Here's a sample SVG:
<!-- my-file.svg -->
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 46 46">
  <g clip-path="url(#test-aaa)">
    <path d="M37.375 42.167H4.792a.958.958 0 0 1-.959-.959V8.626a1.917 1.917 0 1 0-3.833 0v33.542A3.833 3.833 0 0 0 3.833 46h33.542a1.917 1.917 0 0 0 0-3.833z" fill="#99D099"/>
    <rect fill="#008A00" height="39.531" rx="3" width="39.531" x="6.469"/>
    <g clip-path="url(#test-bbb)">
      <path d="M23.15 27.192a1.168 1.168 0 0 0 1.168 1.168h3.114a1.168 1.168 0 0 0 1.168-1.168v-4.866a.195.195 0 0 1 .195-.195h4.866a1.168 1.168 0 0 0 1.169-1.168V17.85a1.168 1.168 0 0 0-1.169-1.168h-4.866a.195.195 0 0 1-.195-.195V11.62a1.168 1.168 0 0 0-1.168-1.168h-3.114a1.168 1.168 0 0 0-1.168 1.168v4.866a.195.195 0 0 1-.195.195H18.09a1.168 1.168 0 0 0-1.168 1.168v3.114a1.168 1.168 0 0 0 1.168 1.168h4.866a.195.195 0 0 1 .195.195v4.866z" fill="#CCE7CC"/>
    </g>
  </g>
  <defs>
    <clipPath id="test-aaa">
      <path d="M0 0h46v46H0z" fill="#fff"/>
    </clipPath>
    <clipPath id="test-bbb">
      <path d="M16.531 10.063h18.688v18.688H16.531z" fill="#fff"/>
    </clipPath>
  </defs>
</svg>
  1. Run the optimizer
  2. See SVG. Ids were minified but not prefixed.

Expected behavior
Expected SVG should be as follows

<!-- my-file.svg -->
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 46 46">
  <g clip-path="url(#my-file_svg__a)">
    <path d="M37.375 42.167H4.792a.958.958 0 0 1-.959-.959V8.626a1.917 1.917 0 1 0-3.833 0v33.542A3.833 3.833 0 0 0 3.833 46h33.542a1.917 1.917 0 0 0 0-3.833z" fill="#99D099"/>
    <rect fill="#008A00" height="39.531" rx="3" width="39.531" x="6.469"/>
    <g clip-path="url(#my-file_svg__b)">
      <path d="M23.15 27.192a1.168 1.168 0 0 0 1.168 1.168h3.114a1.168 1.168 0 0 0 1.168-1.168v-4.866a.195.195 0 0 1 .195-.195h4.866a1.168 1.168 0 0 0 1.169-1.168V17.85a1.168 1.168 0 0 0-1.169-1.168h-4.866a.195.195 0 0 1-.195-.195V11.62a1.168 1.168 0 0 0-1.168-1.168h-3.114a1.168 1.168 0 0 0-1.168 1.168v4.866a.195.195 0 0 1-.195.195H18.09a1.168 1.168 0 0 0-1.168 1.168v3.114a1.168 1.168 0 0 0 1.168 1.168h4.866a.195.195 0 0 1 .195.195v4.866z" fill="#CCE7CC"/>
    </g>
  </g>
  <defs>
    <clipPath id="my-file_svg__a">
      <path d="M0 0h46v46H0z" fill="#fff"/>
    </clipPath>
    <clipPath id="my-file_svg__b">
      <path d="M16.531 10.063h18.688v18.688H16.531z" fill="#fff"/>
    </clipPath>
  </defs>
</svg>

Desktop (please complete the following information):

  • SVGO Version 2.2.0
  • NodeJs Version 12.18.0
  • OS: OSX
@dannobytes dannobytes added the bug label Mar 5, 2021
@polarathene
Copy link

polarathene commented Apr 16, 2021

Without unique prefixes, duplicate IDs will conflict with one another.

My misunderstanding of needing unique ID

Just curious but aren't you able to produce a unique ID, class or depending on context a selector that relies on more than just the svg element for querying (eg div.example > svg {}), IIRC JS can use a selector for finding a element match as well.

Last I knew, relying on IDs was generally frowned upon vs classes, be that for styling or targeting an element with JS.

In some cases where users ask for this, I imagine you would still have the scenario of using the same SVG graphic multiple times but also wanting to isolate them for using JS or CSS to produce variations or different behaviour logic. I'm curious how that's resolved other than giving the <svg> root element a unique class (or ID) and using a selector as you would?

The SVG itself shouldn't be referencing any content outside of it, so is it rather a developer usage concern? (EDIT: It does 😕 ) Or have I misunderstood something here?

eg instead of #my-file_svg__a, you could use #my-file_svg #a

If you're going to use a selector to modify the SVG via JS or styles via CSS, might as well use a better selector. I get that's not always an option though if you're distributing a large library of SVG files for third-parties and you want to minimize the upfront effort for those users.


EDIT: Technically this works on SVGO v1, just not with your example which has a bug.

Static plugin processing order was added in v2.1 and appears to have accidentally mixed the order up (v2.0 also changed plugin order earlier but I'm not sure if was an issue back then too):

svgo/lib/svgo/config.js

Lines 5 to 18 in 3d79f57

const pluginsOrder = [
'removeDoctype',
'removeXMLProcInst',
'removeComments',
'removeMetadata',
'removeXMLNS',
'removeEditorsNSData',
'cleanupAttrs',
'mergeStyles',
'inlineStyles',
'minifyStyles',
'convertStyleToAttrs',
'cleanupIDs',
'prefixIds',

More info (original message while looking into the issue)

Earlier Notes

While I was able to reproduce this, it turned out to be with SVGO v1.x via SVGR. I identified the bug which was specific to id in <defs> being ignored and thus the problem. That was already resolved though and part of the new v2.0 release I think, it didn't seem to get backported to SVG 1.x, and that seems to be due to the project not having separate major version branches, so all v2 work would have been in the same branch or something building up to a v2 release? Unfortunate 🤷‍♂️

The cleanupIDs plugin has a basic "minifier" that doesn't generate a value based on content it's replacing, so you'd not have noticed any difference with prefixIds plugin enabled vs disabled. What happens is as shown with it's list order on the README, your IDs are prefixed first, then cleanupIDs plugin runs... while you expected the order to be the other way around, which would make the most sense...? (EDIT: It appears this is how it originally was in v1)

I'm not sure why the current processing order is like that. I noticed that with the mentioned v1.x bug fix that only made it to v2 did work as you'd expect with prefixIds running after cleanupIDs. v2 changed how plugins are loaded, and then in v2.1 (mid feb) introduced a static order, which introduced the static default order.

cleanupIDs running before prefixIds was likely an accident.

--

Workarounds for the time being

Workarounds

Note that you can specify a prefix in cleanupIDs plugin params:

exports.params = {
remove: true,
minify: true,
prefix: '',
preserve: [],
preservePrefixes: [],
force: false,
};

How useful that is to you depends on how you want to run SVGO, if using something like webpack you can pass the filename context in to adjust the config param for each asset processed. It only accepts a string, not a function like prefixIds plugin. The minifier ID generator is also not configurable 😞

Alternatively in the meantime you should be able to do two passes using slightly different config each: cleanupIDs first. then prefixIds?


Sidenote - Optimizing prefix size

Sidenote - Optimizing prefix size

In looking into this, I'm now better aware of the need for the unique ID regarding internal usage with defs. I had two instances of this MDN gradient example and noticed changing ones gradient colours affected the other when they're both used as separate inline SVG elements in an HTML document.

When you're only interested in avoiding internal conflicts like that, rather than targeting those IDs via selectors for adjustments at runtime, you could potentially have shorter prefixes via a hash on the filename instead, styled-components and others use this for minification:

svgo.config.js:

// DJB2a (XOR variant) is a simple hashing function, reduces input to 32-bits
// Same hashing algorithm as used by `styled-components`.
const SEED = 5381
function djb2a(str) {
  let hash = SEED

  for (let i = 0; i < str.length; i++) {
    hash = (hash * 33) ^ str.charCodeAt(i)
  }

  // Cast to 32-bit uint
  return hash >>> 0
}

// Converts string input to a 32-bit base36 string (0-9, a-z)
// '_' prefix to prevent invalid first chars for CSS class names
const getShortKey = (node, extra) => `_` + djb2a(extra.path).toString(36)

module.exports = {
  datauri: 'unenc',
  js2svg: {
    indent: 2,
    pretty: true,
  },
  plugins: [{
    name: 'prefixIds',
    params: {
      prefix: getShortKey
    }
  }]
}

Would be more optimal as a suffix (you'd still want a delimiter between the minified class/id though).

Using it as a generator function for cleanupIDs instead to further reduce file size by removing the need for a prefix/suffix may sound good but would also be more fragile as this hashing algorithm is a simple one and only covers a range up to 32-bits (a collision after 16-bits aka 65,536 hashes is roughly 50% likely, but with this hash function quite possibly earlier)

EDIT: DJB is pretty bad if it processes short string inputs, I found a very large amount of collisions for a charset of 0-9,a-z,A-Z,_,-(64 chars) at lengths of 2 and 3:

  • 2: Out of 2^12 (4096 permutations), only 802 (roughly 19.6%) unique hashes. This rather bad as a 32-bit hash function should ideally not encounter a collison on average until much later..
  • 3: Out of 2^18 (that's over 250k permutations), only 3.5% unique hashes, the rest had 2 or more inputs deriving the same hash.

That said it seems to fair well on larger string inputs or pure numbers. By contrast another well known simple hashing algorithm (not the fastest or best distribution either) is FNV1a-32, which is a 32-bit hash(there are variants for larger sizes) and it did not have any collisions at all with that same test input.

const fnvOffset = 2166136261
function fnv1a32(str) {
  let h = fnvOffset
  for (let i = 0; i < str.length; i++) {
    h ^= str.charCodeAt(i)
    // JS number type is inaccurate at calculating 'h *= fnvPrime',
    // Uses bit-shifts to accurately multiply the prime: '16777619'
    h += (h << 1) + (h << 4) + (h << 7) + (h << 8) + (h << 24)
  }

  // Cast to 32-bit uint
  return h >>> 0
}

@TrySound
Copy link
Member

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

No branches or pull requests

3 participants