Skip to content

Commit

Permalink
Make visit() more efficient. (#10)
Browse files Browse the repository at this point in the history
  • Loading branch information
johnkenny54 authored Sep 20, 2024
1 parent c7eee27 commit 0d49c61
Show file tree
Hide file tree
Showing 9 changed files with 147 additions and 104 deletions.
2 changes: 1 addition & 1 deletion lib/svgo/coa.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export default function makeProgram(program) {
'Output file or folder (by default the same as the input), "-" for STDOUT',
)
.option(
'--preset <default | none>',
'--preset <default | next | none>',
'Specify which set of predefined plugins to use',
'default',
)
Expand Down
2 changes: 1 addition & 1 deletion lib/version.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
/** Version of SVGO. */
export const VERSION = '5.1.0';
export const VERSION = '5.1.1-alpha.1';
13 changes: 8 additions & 5 deletions lib/xast.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,18 @@ const visitChild = (node, visitor, parents) => {
if (callbacks && callbacks.enter) {
// @ts-ignore
const symbol = callbacks.enter(node, parentNode, parents);
if (symbol === visitSkip) {
// If node was detached, node.parentNode is no longer defined.
if (symbol === visitSkip || !node.parentNode) {
return;
}
}

// visit element children if still attached to parent
if (node.type === 'element') {
parents.push({ element: node });
if (parentNode && parentNode.children.includes(node)) {
for (const child of node.children) {
visitChild(child, visitor, parents);
}
// If node.parentNode was deleted by detachNodeFromParent, don't visit its children.
for (const child of node.children) {
visitChild(child, visitor, parents);
}
parents.pop();
}
Expand All @@ -101,10 +101,13 @@ const visitChild = (node, visitor, parents) => {
* @param {XastChild} node
* @param {XastParent} [parentNode]
*/
// Disable no-unused-vars until all calls to detachNodeFromParent() are updated.
// eslint-disable-next-line no-unused-vars
export const detachNodeFromParent = (node, parentNode) => {
// avoid splice to not break for loops
node.parentNode.children = node.parentNode.children.filter(
(child) => child !== node,
);
// @ts-ignore - plugins should always have parentNode defined; undefine parentNode here so visit() can avoid visiting its children.
delete node.parentNode;
};
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"packageManager": "[email protected]",
"name": "svgo-ll",
"version": "5.1.0",
"version": "5.1.1-alpha.1",
"description": "svgo-ll is a Node.js library and command-line application for optimizing SVG images.",
"license": "MIT",
"type": "module",
Expand Down
12 changes: 10 additions & 2 deletions plugins/removeHiddenElems.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ export const fn = (root, params, info) => {
*/
const removedDefIds = new Set();

/** @type {Set<XastElement>} */
const defNodesToRemove = new Set();

/**
* @type {Set<XastElement>}
*/
Expand Down Expand Up @@ -144,7 +147,7 @@ export const fn = (root, params, info) => {
}

/**
* @param {XastChild} node
* @param {XastElement} node
* @param {XastParent} parentNode
*/
function removeElement(node, parentNode) {
Expand All @@ -157,7 +160,7 @@ export const fn = (root, params, info) => {
removedDefIds.add(node.attributes.id);
}

detachNodeFromParent(node);
defNodesToRemove.add(node);
}

// Record all references in the style element.
Expand Down Expand Up @@ -457,6 +460,11 @@ export const fn = (root, params, info) => {
},
root: {
exit: () => {
for (const child of defNodesToRemove) {
detachNodeFromParent(child);
nonRenderedNodes.delete(child);
}

for (const id of removedDefIds) {
const refs = referencesById.get(id);
if (refs) {
Expand Down
87 changes: 0 additions & 87 deletions test/coa/_index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,6 @@ const EXPECT_TRANS =
'<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><rect x="10" y="20" width="10" height="20" transform="translate(10 20)"/></svg>';
const EXPECT_TRANS_PATH =
'<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><path transform="translate(10 20)" d="M10 20h10v20H10z"/></svg>';
const EXPECT_TRANS_PATH_SORTED =
'<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><path d="M10 20h10v20H10z" transform="translate(10 20)"/></svg>';

describe('test preset option', function () {
afterAll(() => {
Expand Down Expand Up @@ -240,88 +238,3 @@ describe('test preset option', function () {
expect(opt).toBe(EXPECT_TRANS);
});
});

describe('test --enable option', function () {
afterAll(() => {
fs.rmSync(tempFolder, { force: true, recursive: true });
});

it('should only run one plugin with --preset=none and --enable=minifyTransforms', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable=minifyTransforms',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS);
});

it('should run two plugins with --preset=none and --enable minifyTransforms convertShapeToPath minifyPathData', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable',
'minifyTransforms',
'convertShapeToPath',
'minifyPathData',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS_PATH);
});

it('should ignore invalid plugin names', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable',
'x',
'minifyTransforms',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS);
});

it('should work when plugins are specified in custom config', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable',
'convertShapeToPath',
'minifyPathData',
'--config',
path.resolve(PLUGINOPT_DIR, 'config1.js'),
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS_PATH);
});

it('should work with preset-default', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--enable',
'sortAttrs',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS_PATH_SORTED);
});
});
3 changes: 2 additions & 1 deletion test/coa/option.disable.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import svgo from '../../lib/svgo/coa.js';

const __dirname = path.dirname(fileURLToPath(import.meta.url));

const tempFolder = 'temp';
// Add a random number to avoid collisions with other tests.
const tempFolder = 'temp' + Math.random();

/**
* @param {string[]} args
Expand Down
118 changes: 118 additions & 0 deletions test/coa/option.enable.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import fs from 'fs';
import path from 'path';
import { Command } from 'commander';
import { fileURLToPath } from 'url';
import svgo from '../../lib/svgo/coa.js';

const __dirname = path.dirname(fileURLToPath(import.meta.url));

// Add a random number to avoid collisions with other tests.
const tempFolder = 'temp' + Math.random();

/**
* @param {string[]} args
*/
function runProgram(args) {
const program = new Command();
svgo(program);
// prevent running process.exit
program.exitOverride(() => {});
// parser skips first two arguments
return program.parseAsync(['0', '1', ...args]);
}

const PLUGINOPT_DIR = path.resolve(__dirname, 'testPluginOpts');
const PLUGINOPT_FILE1 = path.resolve(PLUGINOPT_DIR, 'test1.svg');
const PLUGINOPT_FILE1_OPT = path.resolve(tempFolder, 'test1.svg');

const EXPECT_TRANS =
'<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><rect x="10" y="20" width="10" height="20" transform="translate(10 20)"/></svg>';
const EXPECT_TRANS_PATH =
'<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><path transform="translate(10 20)" d="M10 20h10v20H10z"/></svg>';
const EXPECT_TRANS_PATH_SORTED =
'<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 100 100"><path d="M10 20h10v20H10z" transform="translate(10 20)"/></svg>';

describe('test --enable option', function () {
afterAll(() => {
fs.rmSync(tempFolder, { force: true, recursive: true });
});

it('should only run one plugin with --preset=none and --enable=minifyTransforms', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable=minifyTransforms',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS);
});

it('should run two plugins with --preset=none and --enable minifyTransforms convertShapeToPath minifyPathData', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable',
'minifyTransforms',
'convertShapeToPath',
'minifyPathData',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS_PATH);
});

it('should ignore invalid plugin names', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable',
'x',
'minifyTransforms',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS);
});

it('should work when plugins are specified in custom config', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--preset=none',
'--enable',
'convertShapeToPath',
'minifyPathData',
'--config',
path.resolve(PLUGINOPT_DIR, 'config1.js'),
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS_PATH);
});

it('should work with preset-default', async () => {
await runProgram([
'-i',
PLUGINOPT_FILE1,
'-o',
PLUGINOPT_FILE1_OPT,
'--quiet',
'--enable',
'sortAttrs',
]);
const opt = fs.readFileSync(PLUGINOPT_FILE1_OPT, { encoding: 'utf8' });
expect(opt).toBe(EXPECT_TRANS_PATH_SORTED);
});
});
12 changes: 6 additions & 6 deletions lib/xast.test.js → test/lib/xast.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/**
* @typedef {import('./types.js').XastElement} XastElement
* @typedef {import('./types.js').XastRoot} XastRoot
* @typedef {import('../../lib/types.js').XastElement} XastElement
* @typedef {import('../../lib/types.js').XastRoot} XastRoot
*/

import { visit, visitSkip, detachNodeFromParent } from './xast.js';
import { visit, visitSkip, detachNodeFromParent } from '../../lib/xast.js';

/**
* @type {(children: XastElement[]) => XastRoot}
Expand All @@ -15,7 +15,7 @@ const root = (children) => {
/**
* @type {(
* name: string,
* parentNode: import('./types.js').XastParent,
* parentNode: import('../../lib/types.js').XastParent,
* attrs?: ?Record<string, string>,
* children?: XastElement[]
* ) => XastElement}
Expand Down Expand Up @@ -100,10 +100,10 @@ test('visit skips entering children if node is detached', () => {
const ast = makeAst();
visit(ast, {
element: {
enter: (node, parentNode) => {
enter: (node) => {
entered.push(node.name);
if (node.name === 'g') {
detachNodeFromParent(node, parentNode);
detachNodeFromParent(node);
}
},
},
Expand Down

0 comments on commit 0d49c61

Please sign in to comment.