prefixIds: { true } generates prefix__prefix__cls-1 class names

[patrick-wc]

Thanks very much to @strarsis for creating the plugin, I was having to manually change class names before using this which was awful. However when I use this plugin with gulp-imagemin I can't get the filename to be passed into the output svg filename.

If I use this config (which from reading around this repo seems to be the way that is suggested, there is no documentation).

.pipe(
        imagemin([
          imagemin.gifsicle({ interlaced: true }),
          imagemin.jpegtran({ progressive: true }),
          imagemin.optipng({ optimizationLevel: 5 }),
          imagemin.svgo({
            plugins: [
              { removeViewBox: true },
              { cleanupIDs: false },
              { prefixIds: true }
            ]
          })
        ])
      )

The output file SVG class names (or IDs) are changed from "cls-1" to "prefix__prefix__cls-1" or in one file I noticed "Layer_2" being changed to "prefix__prefix__prefix__Layer_2"

If I use it with this config, (prefix part copied from @ming-codes )

.pipe(
        imagemin([
          imagemin.gifsicle({ interlaced: true }),
          imagemin.jpegtran({ progressive: true }),
          imagemin.optipng({ optimizationLevel: 5 }),
          imagemin.svgo({
            plugins: [
              { removeViewBox: true },
              { cleanupIDs: false },
              {
                prefixIds: {
                  prefix: {
                    toString() {
                      this.counter = this.counter || 0;

                      return `id-${this.counter++}`;
                    }
                  }
                }
              }
            ]
          })
        ])
      )

The output described above changes from "cls-1" to "id-125__id-111__cls-1" and "id-391__id-373__Layer_2" which although slightly off means I don't get any issues with the same SVGs sharing the same class names which makes them all screw up when inlining them on a page.

It still seems like something is being passed twice as there are repeated values (prefix__prefix__ and id-#__id-#) which is similar to this issue, but I don't think I'm using multipass. I'm not sure why, it's probably my mistake on the configuration arguments but this has taken me a while to get working so I'm hoping this helps someone else.

I am using the latest versions of gulp (4.0.2) and gulp-imagemin (6.1.0) on Windows 10 with Windows Subsystem Linux (WSL to help people out in future when searching).

Please let me know if I'm supposed to use prefixIds with some kind of function call other than true to get the filename coming through, I'm happy to provide more info if needed.

[strarsis]

@patrick-wc: The root of these issues is that there is no built-in plugin testing for CLI and plugin invocations. I have to set up a test case manually and try to reproduce this bug.

Just invoking the svgo CLI works fine:
test.svg:

<svg width="120" height="120" xmlns="http://www.w3.org/2000/svg">
    <rect class="test" x="10" y="10" width="100" height="100"/>
    <rect class="" id="test" x="10" y="10" width="100" height="100"/>
</svg>

./bin/svgo --enable=prefixIds -i ./test.svg -o test.min.svg

test.min.svg:

<svg width="120" height="120" xmlns="http://www.w3.org/2000/svg"><path class="test_svg__test" d="M10 10h100v100H10z"/><path d="M10 10h100v100H10z"/></svg>

The used prefix test_svg__ looks fine, as the input file name is test.svg (dots (.) are replaced by underscores (_)).

I have to set up a manual gulp-based setup and run it over multiple input files so this bug is hopefully reproduced.

[strarsis]

@patrick-wc: So I set up a test in my fork for svgo with gulp-imagemin:

$ git clone https://github.com/strarsis/svgo
$ cd svgo
$ git checkout gulp-test

($ nvm use lts/*) # if node version management is used

$ npm install

Sorry, no better way for replacing svgo used by imagemin-svgo yet:

In file node_modules/imagemin-svgo/index.js replace
const SVGO = require('svgo'); on line 3 with
const SVGO = require('../..');.
Save the changes.

$ cd gulp-test/
$ gulp

Inspect the svg files in gulp-test/dist/images/ folder.

I can confirm that class has apparently been prefixed more than once in the output SVG files.

Can you clone the fork and adjust the setup so that the bug becomes apparent as in your case?

[strarsis]

@patrick-wc: So after some testing with that branch it seems that gulp-imagemin is the reason for the issue: gulp-imagemin passes the SVG content as buffer, without file path/name meta information:
https://github.com/sindresorhus/gulp-imagemin/blob/master/index.js#L78
gulp-imagemin used imagemin which in turn uses imagemin-svgo and it also just passes the SVG content as buffer, without file related meta data:
https://github.com/imagemin/imagemin-svgo/blob/master/index.js#L17
So svgo can't know the file path/name, hence it can't generate a prefix from it.

gulp encapsulates files in stream as vinyl objects, the SVG contents are passed as buffer to imagemin and eventually to svgo, without file related meta data.

gulp-svgo which doesn't use imagemin as abstraction, actually passes the file path to svgo:
https://github.com/corneliusio/gulp-svgo/blob/master/index.js#L21

So there are three ways for making this work:

Make file path pass through gulp-imagemin, imagemin and imagemin-svgo

1. Architectural discussions in imagemin issues.
2. Creation of PR with the agreed-on additions for imagemin.
3. Merging of PR to imagemin.
4. Release of imagemin to npm.
5. Then PR for gulp-imagemin.
6. Merging of PR to gulp-imagemin.
7. Release of gulp-imagemin to npm.
8. Finally PR for imagemin-svgo.
9. Merging of PR to imagemin-svgo.
10. Release of imagemin-svgo to npm.
11. End-dev (you) has to update gulp-imagemin and it should work then.

Somehow put the file path and other metadata into the SVG (XML) content, e.g. as comment

This would work without changes to gulp-imagemin, imagemin and imagemin-svgo.
svgo core or a dedicated plugin, that runs first, would extract the metadata from the SVG XML, e.g. by parsing a special comment, to late-initialize some svgo options and clean it from the XML afterwards.

Use svgo plugin prefixIds as extra plugin using gulp-svgo

This means to add an extra gulp task using gulp-svgo with only prefixIds plugin before the gulp-imagemin task. with prefixIds plugin disabled.
This would require the end-dev (you) to additionally install gulp-svgo and configure it to only use prefixIds plugin. This solution can be used right now.

const gulpSvgo = require('gulp-svgo');
const imagemin = require('gulp-imagemin');

      .pipe(
            gulpSvgo({
                  plugins: [
                        { prefixIds: true } // only this should be enabled for gulp-svgo!
                        // But how to disable all other plugins?`Typing them all out wouldn't be robust...
                  ]
            })
      )
      .pipe(
        imagemin([
          imagemin.gifsicle({ interlaced: true }),
          imagemin.jpegtran({ progressive: true }),
          imagemin.optipng({ optimizationLevel: 5 }),
          imagemin.svgo({
            plugins: [
              { removeViewBox: true },
              { cleanupIDs: false },
              { prefixIds: false } // disable it for gulp-imagemin!
            ]
          })
        ])
      )

[wesselbaum]

Wouldn't it be sufficient checking if the prefix which will be added already exists in the unprefixed id/class?

[strarsis]

@wesselbaum: I still have to find out why there are SVGs are passed to svgo with prefixes.
The SVGs should be only passed as original ones once.

[wesselbaum]

I won't disagree on this one for this issue.
I've got similar use case, but I use svgo in my project for inline-SVGs and if there is a new SVG file I wanted to run one script which cleans up all SVGs. This approach won't work with the current prefixIds since after a couple runs the IDs will be very long.
Long story short: Could you also consider my use case and maybe my solution?

[strarsis]

@wesselbaum: Yes, it seems reasonable. There is a catch though: What if, by chance, the classes start with something that should be also as the prefix? If in that case the prefix is skipped, there is no guarantee that there are no collisions between other SVG files that had no prefix and are then prefixed. So what do you think about an extra option that can be still turned off, skipPrefixed, that would make the prefix plugin skip all classes/IDs/links that already start with the prefix to be applied?

Edit: This explains why a SVG is passed multiple files to svgo, imagemin-svgo enables the multipass option by default: https://github.com/imagemin/imagemin-svgo/blob/master/index.js#L6
gulp-svgo doesn't enable it by the way.

[wesselbaum]

Sounds good to me. I appreciate your work a lot. Thanks!

[strarsis]

@wesselbaum:
You can try the fix out already without waiting it to be reviewed + merged
by installing from its GitHub fork:

npm install github:strarsis:add-multipass-info

Depending on how your Gulp setup uses svgo, it may not override the svgo used by gulp-imagemin, imagemin and imagemin-svgo. You may have to go node_modules/imagemin-svgo and install the svgo there to override the svgo in node_modules/svgo inside.

Please try this fix and report whether it works in your setup.

[wesselbaum]

Sadly I can't try it until next week but I will report my results as soon as possible. Thanks.

[strarsis]

I just noticed that flagging each processed node isn't the best solution.
It would be better if svgo passes a multipass key to extra argument of each plugin.

[wesselbaum]

You can try the fix out already without waiting it to be reviewed + merged
by installing from its GitHub fork:

npm install github:strarsis/svgo#fix-prefixids-multipass

The skipPrefixed solution is not available any more, right? The solution above doesn't work for my problem. You wrote that it could lead to collisions, but isn't it okay if it is an opt in solution?

[strarsis]

@wesselbaum: A new approach is used now that adds multipassCount to
the info object that is passed to the plugins: #1173
This allows the plugins to know about subsequent passes and skip them when necessary,
the prefixIds plugin uses that information in the PR to skip and prevent multiple prefixing.
Luckily, multipass is handled internally by svgo.

For trying out the fix (PR) before it is merged, see my previous post with updated instructions.

[dphil]

@strarsis There appears to be a bug with #1173. The way multipassCount is initialized causes prefixIds to still run twice. I have proposed a fix in #1206.

[dphil]

Ah, I just found the open PR #1177 where you already caught this, @strarsis . I have closed mine as a duplicate.

[TrySound]

Fixed in 2.0.1