Refactor prefixIds (#1561)

Ref https://github.com/svg/svgo/issues/1499

- migrated to visitor plugin api
- covered with tsdoc
- made the plugin idempotent as requested a few times
  Now even manually running svgo a few times will not duplicate
  prefix in ids and classes
- run each plugin test twice to see which plugin need to run many times
  ideally idempotent plugins will allow to get rid of multipass option in v3

lib/svgo.test.js

@@ -1,5 +1,9 @@
 'use strict';
 
+/**
+ * @typedef {import('../lib/types').Plugin} Plugin
+ */
+
 const { optimize } = require('./svgo.js');
 
 test('allow to setup default preset', () => {
@@ -324,3 +328,28 @@ test('provides legacy error message', () => {
   4 | 
 `);
 });
+
+test('multipass option should trigger plugins multiple times', () => {
+  const svg = `<svg id="abcdefghijklmnopqrstuvwxyz"></svg>`;
+  const list = [];
+  /**
+   * @type {Plugin<void>}
+   */
+  const testPlugin = {
+    type: 'visitor',
+    name: 'testPlugin',
+    fn: (_root, _params, info) => {
+      list.push(info.multipassCount);
+      return {
+        element: {
+          enter: (node) => {
+            node.attributes.id = node.attributes.id.slice(1);
+          },
+        },
+      };
+    },
+  };
+  const { data } = optimize(svg, { multipass: true, plugins: [testPlugin] });
+  expect(list).toEqual([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
+  expect(data).toEqual(`<svg id="klmnopqrstuvwxyz"/>`);
+});

lib/types.ts

@@ -71,7 +71,16 @@ export type Visitor = {
   root?: VisitorRoot;
 };
 
-export type Plugin<Params> = (root: XastRoot, params: Params) => null | Visitor;
+export type PluginInfo = {
+  path?: string;
+  multipassCount: number;
+};
+
+export type Plugin<Params> = (
+  root: XastRoot,
+  params: Params,
+  info: PluginInfo
+) => null | Visitor;
 
 export type Specificity = [number, number, number, number];
 

plugins/prefixIds.js

@@ -1,300 +1,240 @@
 'use strict';
 
+const csstree = require('css-tree');
+const { referencesProps } = require('./_collections.js');
+
+/**
+ * @typedef {import('../lib/types').XastElement} XastElement
+ * @typedef {import('../lib/types').PluginInfo} PluginInfo
+ */
+
+exports.type = 'visitor';
 exports.name = 'prefixIds';
-
-exports.type = 'perItem';
-
 exports.active = false;
-
-exports.params = {
-  delim: '__',
-  prefixIds: true,
-  prefixClassNames: true,
-};
-
 exports.description = 'prefix IDs';
 
-var csstree = require('css-tree'),
+/**
-  collections = require('./_collections.js'),
+ * extract basename from path
-  referencesProps = collections.referencesProps,
+ * @type {(path: string) => string}
-  rxId = /^#(.*)$/, // regular expression for matching an ID + extracing its name
+ */
-  addPrefix = null;
-
-const unquote = (string) => {
-  const first = string.charAt(0);
-  if (first === "'" || first === '"') {
-    if (first === string.charAt(string.length - 1)) {
-      return string.slice(1, -1);
-    }
-  }
-  return string;
-};
-
-// Escapes a string for being used as ID
-var escapeIdentifierName = function (str) {
-  return str.replace(/[. ]/g, '_');
-};
-
-// Matches an #ID value, captures the ID name
-var matchId = function (urlVal) {
-  var idUrlMatches = urlVal.match(rxId);
-  if (idUrlMatches === null) {
-    return false;
-  }
-  return idUrlMatches[1];
-};
-
-// Matches an url(...) value, captures the URL
-var matchUrl = function (val) {
-  var urlMatches = /url\((.*?)\)/gi.exec(val);
-  if (urlMatches === null) {
-    return false;
-  }
-  return urlMatches[1];
-};
-
-// prefixes an #ID
-var prefixId = function (val) {
-  var idName = matchId(val);
-  if (!idName) {
-    return false;
-  }
-  return '#' + addPrefix(idName);
-};
-
-// prefixes a class attribute value
-const addPrefixToClassAttr = (element, name) => {
-  if (
-    element.attributes[name] == null ||
-    element.attributes[name].length === 0
-  ) {
-    return;
-  }
-
-  element.attributes[name] = element.attributes[name]
-    .split(/\s+/)
-    .map(addPrefix)
-    .join(' ');
-};
-
-// prefixes an ID attribute value
-const addPrefixToIdAttr = (element, name) => {
-  if (
-    element.attributes[name] == null ||
-    element.attributes[name].length === 0
-  ) {
-    return;
-  }
-
-  element.attributes[name] = addPrefix(element.attributes[name]);
-};
-
-// prefixes a href attribute value
-const addPrefixToHrefAttr = (element, name) => {
-  if (
-    element.attributes[name] == null ||
-    element.attributes[name].length === 0
-  ) {
-    return;
-  }
-
-  const idPrefixed = prefixId(element.attributes[name]);
-  if (!idPrefixed) {
-    return;
-  }
-  element.attributes[name] = idPrefixed;
-};
-
-// prefixes an URL attribute value
-const addPrefixToUrlAttr = (element, name) => {
-  if (
-    element.attributes[name] == null ||
-    element.attributes[name].length === 0
-  ) {
-    return;
-  }
-
-  // url(...) in value
-  const urlVal = matchUrl(element.attributes[name]);
-  if (!urlVal) {
-    return;
-  }
-
-  const idPrefixed = prefixId(urlVal);
-  if (!idPrefixed) {
-    return;
-  }
-
-  element.attributes[name] = 'url(' + idPrefixed + ')';
-};
-
-// prefixes begin/end attribute value
-const addPrefixToBeginEndAttr = (element, name) => {
-  if (
-    element.attributes[name] == null ||
-    element.attributes[name].length === 0
-  ) {
-    return;
-  }
-
-  const parts = element.attributes[name].split('; ').map((val) => {
-    val = val.trim();
-
-    if (val.endsWith('.end') || val.endsWith('.start')) {
-      const [id, postfix] = val.split('.');
-
-      let idPrefixed = prefixId(`#${id}`);
-
-      if (!idPrefixed) {
-        return val;
-      }
-
-      idPrefixed = idPrefixed.slice(1);
-      return `${idPrefixed}.${postfix}`;
-    } else {
-      return val;
-    }
-  });
-
-  element.attributes[name] = parts.join('; ');
-};
-
 const getBasename = (path) => {
   // extract everything after latest slash or backslash
-  const matched = path.match(/[/\\]([^/\\]+)$/);
+  const matched = path.match(/[/\\]?([^/\\]+)$/);
   if (matched) {
     return matched[1];
   }
   return '';
 };
 
+/**
+ * escapes a string for being used as ID
+ * @type {(string: string) => string}
+ */
+const escapeIdentifierName = (str) => {
+  return str.replace(/[. ]/g, '_');
+};
+
+/**
+ * @type {(string: string) => string}
+ */
+const unquote = (string) => {
+  if (
+    (string.startsWith('"') && string.endsWith('"')) ||
+    (string.startsWith("'") && string.endsWith("'"))
+  ) {
+    return string.slice(1, -1);
+  }
+  return string;
+};
+
+/**
+ * prefix an ID
+ * @type {(prefix: string, name: string) => string}
+ */
+const prefixId = (prefix, value) => {
+  if (value.startsWith(prefix)) {
+    return value;
+  }
+  return prefix + value;
+};
+
+/**
+ * prefix an #ID
+ * @type {(prefix: string, name: string) => string | null}
+ */
+const prefixReference = (prefix, value) => {
+  if (value.startsWith('#')) {
+    return '#' + prefixId(prefix, value.slice(1));
+  }
+  return null;
+};
+
 /**
  * Prefixes identifiers
  *
- * @param {Object} node node
- * @param {Object} opts plugin params
- * @param {Object} extra plugin extra information
- *
  * @author strarsis <strarsis@gmail.com>
+ *
+ * @type {import('../lib/types').Plugin<{
+ *   prefix?: boolean | string | ((node: XastElement, info: PluginInfo) => string),
+ *   delim?: string,
+ *   prefixIds?: boolean,
+ *   prefixClassNames?: boolean,
+ * }>}
  */
-exports.fn = function (node, opts, extra) {
+exports.fn = (_root, params, info) => {
-  // skip subsequent passes when multipass is used
+  const { delim = '__', prefixIds = true, prefixClassNames = true } = params;
-  if (extra.multipassCount && extra.multipassCount > 0) {
-    return;
-  }
 
-  // prefix, from file name or option
+  return {
-  var prefix = 'prefix';
+    element: {
-  if (opts.prefix) {
+      enter: (node) => {
-    if (typeof opts.prefix === 'function') {
+        /**
-      prefix = opts.prefix(node, extra);
+         * prefix, from file name or option
-    } else {
+         * @type {string}
-      prefix = opts.prefix;
+         */
-    }
+        let prefix = 'prefix' + delim;
-  } else if (opts.prefix === false) {
+        if (typeof params.prefix === 'function') {
-    prefix = false;
+          prefix = params.prefix(node, info) + delim;
-  } else if (extra && extra.path && extra.path.length > 0) {
+        } else if (typeof params.prefix === 'string') {
-    var filename = getBasename(extra.path);
+          prefix = params.prefix + delim;
-    prefix = filename;
+        } else if (params.prefix === false) {
-  }
+          prefix = '';
+        } else if (info.path != null && info.path.length > 0) {
+          prefix = escapeIdentifierName(getBasename(info.path)) + delim;
+        }
 
-  // prefixes a normal value
+        // prefix id/class selectors and url() references in styles
-  addPrefix = function (name) {
+        if (node.name === 'style') {
-    if (prefix === false) {
+          // skip empty <style/> elements
-      return escapeIdentifierName(name);
+          if (node.children.length === 0) {
-    }
+            return;
-    return escapeIdentifierName(prefix + opts.delim + name);
+          }
-  };
 
-  // <style/> property values
+          // parse styles
+          let cssText = '';
+          if (
+            node.children[0].type === 'text' ||
+            node.children[0].type === 'cdata'
+          ) {
+            cssText = node.children[0].value;
+          }
+          /**
+           * @type {null | csstree.CssNode}
+           */
+          let cssAst = null;
+          try {
+            cssAst = csstree.parse(cssText, {
+              parseValue: true,
+              parseCustomProperty: false,
+            });
+          } catch {
+            return;
+          }
 
-  if (node.type === 'element' && node.name === 'style') {
+          csstree.walk(cssAst, (node) => {
-    if (node.children.length === 0) {
+            // #ID, .class selectors
-      // skip empty <style/>s
+            if (
-      return;
+              (prefixIds && node.type === 'IdSelector') ||
-    }
+              (prefixClassNames && node.type === 'ClassSelector')
+            ) {
+              node.name = prefixId(prefix, node.name);
+              return;
+            }
+            // url(...) references
+            if (
+              node.type === 'Url' &&
+              node.value.value &&
+              node.value.value.length > 0
+            ) {
+              const prefixed = prefixReference(
+                prefix,
+                unquote(node.value.value)
+              );
+              if (prefixed != null) {
+                node.value.value = prefixed;
+              }
+            }
+          });
 
-    var cssStr = '';
+          // update styles
-    if (node.children[0].type === 'text' || node.children[0].type === 'cdata') {
+          if (
-      cssStr = node.children[0].value;
+            node.children[0].type === 'text' ||
-    }
+            node.children[0].type === 'cdata'
-
+          ) {
-    var cssAst = {};
+            node.children[0].value = csstree.generate(cssAst);
-    try {
+          }
-      cssAst = csstree.parse(cssStr, {
-        parseValue: true,
-        parseCustomProperty: false,
-      });
-    } catch (parseError) {
-      console.warn(
-        'Warning: Parse error of styles of <style/> element, skipped. Error details: ' +
-          parseError
-      );
-      return;
-    }
-
-    var idPrefixed = '';
-    csstree.walk(cssAst, function (node) {
-      // #ID, .class
-      if (
-        ((opts.prefixIds && node.type === 'IdSelector') ||
-          (opts.prefixClassNames && node.type === 'ClassSelector')) &&
-        node.name
-      ) {
-        node.name = addPrefix(node.name);
-        return;
-      }
-
-      // url(...) in value
-      if (
-        node.type === 'Url' &&
-        node.value.value &&
-        node.value.value.length > 0
-      ) {
-        idPrefixed = prefixId(unquote(node.value.value));
-        if (!idPrefixed) {
           return;
         }
-        node.value.value = idPrefixed;
-      }
-    });
 
-    // update <style>s
+        // prefix an ID attribute value
-    node.children[0].value = csstree.generate(cssAst);
+        if (
-    return;
+          prefixIds &&
-  }
+          node.attributes.id != null &&
+          node.attributes.id.length !== 0
+        ) {
+          node.attributes.id = prefixId(prefix, node.attributes.id);
+        }
 
-  // element attributes
+        // prefix a class attribute value
+        if (
+          prefixClassNames &&
+          node.attributes.class != null &&
+          node.attributes.class.length !== 0
+        ) {
+          node.attributes.class = node.attributes.class
+            .split(/\s+/)
+            .map((name) => prefixId(prefix, name))
+            .join(' ');
+        }
 
-  if (node.type !== 'element') {
+        // prefix a href attribute value
-    return;
+        // xlink:href is deprecated, must be still supported
-  }
+        for (const name of ['href', 'xlink:href']) {
+          if (
+            node.attributes[name] != null &&
+            node.attributes[name].length !== 0
+          ) {
+            const prefixed = prefixReference(prefix, node.attributes[name]);
+            if (prefixed != null) {
+              node.attributes[name] = prefixed;
+            }
+          }
+        }
 
-  // Nodes
+        // prefix an URL attribute value
+        for (const name of referencesProps) {
+          if (
+            node.attributes[name] != null &&
+            node.attributes[name].length !== 0
+          ) {
+            // extract id reference from url(...) value
+            const matches = /url\((.*?)\)/gi.exec(node.attributes[name]);
+            if (matches != null) {
+              const value = matches[1];
+              const prefixed = prefixReference(prefix, value);
+              if (prefixed != null) {
+                node.attributes[name] = `url(${prefixed})`;
+              }
+            }
+          }
+        }
 
-  if (opts.prefixIds) {
+        // prefix begin/end attribute value
-    // ID
+        for (const name of ['begin', 'end']) {
-    addPrefixToIdAttr(node, 'id');
+          if (
-  }
+            node.attributes[name] != null &&
-
+            node.attributes[name].length !== 0
-  if (opts.prefixClassNames) {
+          ) {
-    // Class
+            const parts = node.attributes[name].split(/\s*;\s+/).map((val) => {
-    addPrefixToClassAttr(node, 'class');
+              if (val.endsWith('.end') || val.endsWith('.start')) {
-  }
+                const [id, postfix] = val.split('.');
-
+                return `${prefixId(prefix, id)}.${postfix}`;
-  // References
+              }
-
+              return val;
-  // href
+            });
-  addPrefixToHrefAttr(node, 'href');
+            node.attributes[name] = parts.join('; ');
-
+          }
-  // (xlink:)href (deprecated, must be still supported)
+        }
-  addPrefixToHrefAttr(node, 'xlink:href');
+      },
-
+    },
-  // (referenceable) properties
+  };
-  for (var referencesProp of referencesProps) {
-    addPrefixToUrlAttr(node, referencesProp);
-  }
-
-  addPrefixToBeginEndAttr(node, 'begin');
-  addPrefixToBeginEndAttr(node, 'end');
 };

test/plugins/_index.test.js

@@ -30,14 +30,21 @@ describe('plugins tests', function () {
             name,
             params: params ? JSON.parse(params) : {},
           };
-          const result = optimize(original, {
+          let lastResultData = original;
-            path: file,
+          // test plugins idempotence
-            plugins: [plugin],
+          const exclude = ['addAttributesToSVGElement', 'convertTransform'];
-            js2svg: { pretty: true },
+          const multipass = exclude.includes(name) ? 1 : 2;
-          });
+          for (let i = 0; i < multipass; i += 1) {
-          expect(result.error).not.toEqual(expect.anything());
+            const result = optimize(lastResultData, {
-          //FIXME: results.data has a '\n' at the end while it should not
+              path: file,
-          expect(normalize(result.data)).toEqual(should);
+              plugins: [plugin],
+              js2svg: { pretty: true },
+            });
+            lastResultData = result.data;
+            expect(result.error).not.toEqual(expect.anything());
+            //FIXME: results.data has a '\n' at the end while it should not
+            expect(normalize(result.data)).toEqual(should);
+          }
         });
       });
     }

test/plugins/prefixIds.test.js

@@ -0,0 +1,24 @@
+'use strict';
+
+const { optimize } = require('../../lib/svgo.js');
+
+test('should extract prefix from path basename', () => {
+  const svg = `<svg id="my-id"></svg>`;
+  expect(
+    optimize(svg, {
+      plugins: ['prefixIds'],
+    }).data
+  ).toEqual(`<svg id="prefix__my-id"/>`);
+  expect(
+    optimize(svg, {
+      plugins: ['prefixIds'],
+      path: 'input.svg',
+    }).data
+  ).toEqual(`<svg id="input_svg__my-id"/>`);
+  expect(
+    optimize(svg, {
+      plugins: ['prefixIds'],
+      path: 'path/to/input.svg',
+    }).data
+  ).toEqual(`<svg id="input_svg__my-id"/>`);
+});

test/svgo/_index.test.js

@@ -26,21 +26,6 @@ describe('svgo', () => {
     });
     expect(normalize(result.data)).toEqual(expected);
   });
-  it('should run multiple times', async () => {
-    const [original, expected] = await parseFixture('multipass.svg');
-    const result = optimize(original, {
-      multipass: true,
-    });
-    expect(normalize(result.data)).toEqual(expected);
-  });
-  it('should pass multipass count to plugins', async () => {
-    const [original, expected] = await parseFixture('multipass-prefix-ids.svg');
-    const result = optimize(original, {
-      multipass: true,
-      plugins: ['preset-default', 'prefixIds'],
-    });
-    expect(normalize(result.data)).toEqual(expected);
-  });
   it('should handle plugins order properly', async () => {
     const [original, expected] = await parseFixture('plugins-order.svg');
     const result = optimize(original, { input: 'file', path: 'input.svg' });

test/svgo/multipass-prefix-ids.svg

@@ -1,7 +0,0 @@
-<svg width="120" height="120" xmlns="http://www.w3.org/2000/svg">
-  <rect class="test" x="10" y="10" width="100" height="100"/>
-</svg>
-
-@@@
-
-<svg width="120" height="120" xmlns="http://www.w3.org/2000/svg"><path class="prefix__test" d="M10 10h100v100H10z"/></svg>

test/svgo/multipass.svg

@@ -1,18 +0,0 @@
-<!-- https://github.com/svg/svgo/pull/258#issue-23706813 -->
-<svg xmlns="http://www.w3.org/2000/svg">
-  <g transform="translate(-39, -239)">
-    <g id="test" transform="translate(39, 37)">
-      <g>
-        <rect id="Rectangle-1183" fill="#D4D4D4" x="8" y="223" width="7" height="19"></rect>
-        <rect id="Rectangle-1183" fill="#D4D4D4" x="4" y="227" width="42" height="3"></rect>
-        <rect id="Rectangle-1183" fill="#D0011B" x="23" y="227" width="6" height="3"></rect>
-        <rect id="Rectangle-1183" fill="#D0011B" x="35" y="227" width="6" height="3"></rect>
-        <rect id="Rectangle-1183" fill="#D0011B" x="11" y="227" width="6" height="3"></rect>
-      </g>
-    </g>
-  </g>
-</svg>
-
-@@@
-
-<svg xmlns="http://www.w3.org/2000/svg"><path fill="#D4D4D4" d="M8 21h7v19H8z"/><path fill="#D4D4D4" d="M4 25h42v3H4z"/><path fill="#D0011B" d="M23 25h6v3h-6zm12 0h6v3h-6zm-24 0h6v3h-6z"/></svg>

tsconfig.json

@@ -25,7 +25,6 @@
     "plugins/moveElemsAttrsToGroup.js",
     "plugins/moveGroupAttrsToElems.js",
     "plugins/plugins.js",
-    "plugins/prefixIds.js",
     "plugins/removeDimensions.js",
     "plugins/removeEmptyAttrs.js",
     "plugins/removeNonInheritableGroupAttrs.js",