From d08815c1cd3ad087c536c24868fcd9299cb4da31 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 22 Mar 2021 18:55:08 +0300 Subject: [PATCH] Implement simple node clone (#1450) https://github.com/svg/svgo/pull/1279 faced some problems with node cloning. In this diff I moved class/style live objects to JSAPI and made it non enumerable to avoid deep cloning issues. class list and style declaration classes are not longer need own clone methods. --- lib/svgo/css-class-list.js | 37 ++++----------------- lib/svgo/css-style-declaration.js | 36 ++++----------------- lib/svgo/jsAPI.js | 54 +++++++++++++------------------ lib/svgo/svg2js.js | 46 +++++++++----------------- 4 files changed, 51 insertions(+), 122 deletions(-) diff --git a/lib/svgo/css-class-list.js b/lib/svgo/css-class-list.js index 08d941678..33c515668 100644 --- a/lib/svgo/css-class-list.js +++ b/lib/svgo/css-class-list.js @@ -3,34 +3,11 @@ var CSSClassList = function (node) { this.parentNode = node; this.classNames = new Set(); - //this.classValue = null; -}; - -/** - * Performs a deep clone of this object. - * - * @param parentNode the parentNode to assign to the cloned result - */ -CSSClassList.prototype.clone = function (parentNode) { - var node = this; - var nodeData = {}; - - Object.keys(node).forEach(function (key) { - if (key !== 'parentNode') { - nodeData[key] = node[key]; - } - }); - - // Deep-clone node data. - nodeData = JSON.parse(JSON.stringify(nodeData)); - - var clone = new CSSClassList(parentNode); - Object.assign(clone, nodeData); - return clone; -}; - -CSSClassList.prototype.hasClass = function () { - this.addClassValueHandler(); + const value = node.attributes.class; + if (value != null) { + this.addClassValueHandler(); + this.setClassValue(value); + } }; // attr.class.value @@ -59,7 +36,7 @@ CSSClassList.prototype.setClassValue = function (newValue) { }; CSSClassList.prototype.add = function (/* variadic */) { - this.hasClass(); + this.addClassValueHandler(); Object.values(arguments).forEach(this._addSingle.bind(this)); }; @@ -68,7 +45,7 @@ CSSClassList.prototype._addSingle = function (className) { }; CSSClassList.prototype.remove = function (/* variadic */) { - this.hasClass(); + this.addClassValueHandler(); Object.values(arguments).forEach(this._removeSingle.bind(this)); }; diff --git a/lib/svgo/css-style-declaration.js b/lib/svgo/css-style-declaration.js index cf5169429..c5fb30c43 100644 --- a/lib/svgo/css-style-declaration.js +++ b/lib/svgo/css-style-declaration.js @@ -12,33 +12,11 @@ var CSSStyleDeclaration = function (node) { this.styleValue = null; this.parseError = false; -}; - -/** - * Performs a deep clone of this object. - * - * @param parentNode the parentNode to assign to the cloned result - */ -CSSStyleDeclaration.prototype.clone = function (parentNode) { - var node = this; - var nodeData = {}; - - Object.keys(node).forEach(function (key) { - if (key !== 'parentNode') { - nodeData[key] = node[key]; - } - }); - - // Deep-clone node data. - nodeData = JSON.parse(JSON.stringify(nodeData)); - - var clone = new CSSStyleDeclaration(parentNode); - Object.assign(clone, nodeData); - return clone; -}; - -CSSStyleDeclaration.prototype.hasStyle = function () { - this.addStyleValueHandler(); + const value = node.attributes.style; + if (value != null) { + this.addStyleValueHandler(); + this.setStyleValue(value); + } }; // attr.style.value @@ -210,7 +188,7 @@ CSSStyleDeclaration.prototype.removeProperty = function (propertyName) { throw Error('1 argument required, but only 0 present.'); } - this.hasStyle(); + this.addStyleValueHandler(); var properties = this.getProperties(); this._handleParseError(); @@ -237,7 +215,7 @@ CSSStyleDeclaration.prototype.setProperty = function ( throw Error('propertyName argument required, but only not present.'); } - this.hasStyle(); + this.addStyleValueHandler(); var properties = this.getProperties(); this._handleParseError(); diff --git a/lib/svgo/jsAPI.js b/lib/svgo/jsAPI.js index 1894964be..89eeade95 100644 --- a/lib/svgo/jsAPI.js +++ b/lib/svgo/jsAPI.js @@ -3,6 +3,8 @@ const { selectAll, selectOne, is } = require('css-select'); const { parseName } = require('./tools.js'); const svgoCssSelectAdapter = require('./css-select-adapter'); +const CSSClassList = require('./css-class-list'); +const CSSStyleDeclaration = require('./css-style-declaration'); var cssSelectOpts = { xmlMode: true, @@ -39,6 +41,21 @@ var JSAPI = function (data, parentNode) { if (this.children == null) { this.children = []; } + Object.defineProperty(this, 'class', { + writable: true, + configurable: true, + value: new CSSClassList(this), + }); + Object.defineProperty(this, 'style', { + writable: true, + configurable: true, + value: new CSSStyleDeclaration(this), + }); + Object.defineProperty(this, 'parentNode', { + writable: true, + value: parentNode, + }); + // temporary attrs polyfill // TODO remove after migration const element = this; @@ -56,12 +73,6 @@ var JSAPI = function (data, parentNode) { }, }); } - if (parentNode) { - Object.defineProperty(this, 'parentNode', { - writable: true, - value: parentNode, - }); - } }; module.exports = JSAPI; @@ -71,37 +82,16 @@ module.exports = JSAPI; * @return {Object} element */ JSAPI.prototype.clone = function () { - var node = this; - var nodeData = {}; - - Object.keys(node).forEach(function (key) { - if (key !== 'class' && key !== 'style' && key !== 'children') { - nodeData[key] = node[key]; - } - }); - + const { children, ...nodeData } = this; // Deep-clone node data. - nodeData = JSON.parse(JSON.stringify(nodeData)); - - // parentNode gets set to a proper object by the parent clone, - // but it needs to be true/false now to do the right thing - // in the constructor. - var clonedNode = new JSAPI(nodeData, !!node.parentNode); - - if (node.class) { - clonedNode.class = node.class.clone(clonedNode); - } - if (node.style) { - clonedNode.style = node.style.clone(clonedNode); - } - if (node.children) { - clonedNode.children = node.children.map(function (childNode) { - var clonedChild = childNode.clone(); + const clonedNode = new JSAPI(JSON.parse(JSON.stringify(nodeData)), null); + if (children) { + clonedNode.children = children.map((child) => { + const clonedChild = child.clone(); clonedChild.parentNode = clonedNode; return clonedChild; }); } - return clonedNode; }; diff --git a/lib/svgo/svg2js.js b/lib/svgo/svg2js.js index 25f25679c..b150a5ca9 100644 --- a/lib/svgo/svg2js.js +++ b/lib/svgo/svg2js.js @@ -1,13 +1,12 @@ 'use strict'; -var SAX = require('@trysound/sax'), - JSAPI = require('./jsAPI.js'), - CSSClassList = require('./css-class-list'), - CSSStyleDeclaration = require('./css-style-declaration'), - textElems = require('../../plugins/_collections.js').textElems, - entityDeclaration = //g; - -var config = { +const SAX = require('@trysound/sax'); +const JSAPI = require('./jsAPI.js'); +const { textElems } = require('../../plugins/_collections.js'); + +const entityDeclaration = //g; + +const config = { strict: true, trim: false, normalize: false, @@ -22,10 +21,10 @@ var config = { * @param {String} data input data */ module.exports = function (data) { - var sax = SAX.parser(config.strict, config), - root = new JSAPI({ type: 'root', children: [] }), - current = root, - stack = [root]; + const sax = SAX.parser(config.strict, config); + const root = new JSAPI({ type: 'root', children: [] }); + let current = root; + let stack = [root]; function pushToContent(node) { const wrapped = new JSAPI(node, current); @@ -43,8 +42,8 @@ module.exports = function (data) { }, }); - var subsetStart = doctype.indexOf('['), - entityMatch; + const subsetStart = doctype.indexOf('['); + let entityMatch; if (subsetStart >= 0) { entityDeclaration.lastIndex = subsetStart; @@ -85,23 +84,8 @@ module.exports = function (data) { children: [], }; - element.class = new CSSClassList(element); - element.style = new CSSStyleDeclaration(element); - - if (Object.keys(data.attributes).length) { - for (const [name, attr] of Object.entries(data.attributes)) { - if (name === 'class') { - // has class attribute - element.class.hasClass(); - } - - if (name === 'style') { - // has style attribute - element.style.hasStyle(); - } - - element.attributes[name] = attr.value; - } + for (const [name, attr] of Object.entries(data.attributes)) { + element.attributes[name] = attr.value; } element = pushToContent(element);