Implement simple node clone (#1450)

#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.
svg · Mar 22, 2021 · d08815c · d08815c
1 parent 3d4adb6
commit d08815c
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 122 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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 = /<!ENTITY\s+(\S+)\s+(?:'([^']+)'|"([^"]+)")\s*>/g;
-
-var config = {
+const SAX = require('@trysound/sax');
+const JSAPI = require('./jsAPI.js');
+const { textElems } = require('../../plugins/_collections.js');
+
+const entityDeclaration = /<!ENTITY\s+(\S+)\s+(?:'([^']+)'|"([^"]+)")\s*>/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);