Pass parent node to visitor (#1517)

Wrapping each node with JSAPI class and passing parent to it is not
reliable. Parent may be changed but the reference will stay.

Here I wasn't able to detach comment from parent node for some reason.
Explicit parent node inferred while ast traverse is easier to debug and
work with. Eventually we will not need to wrap each node with JSAPI class.

lib/xast.js

@@ -52,34 +52,33 @@ const traverse = (node, fn) => {
 };
 exports.traverse = traverse;
 
-const visit = (node, visitor) => {
+const visit = (node, visitor, parentNode = null) => {
   const callbacks = visitor[node.type];
   if (callbacks && callbacks.enter) {
-    callbacks.enter(node);
+    callbacks.enter(node, parentNode);
   }
   // visit root children
   if (node.type === 'root') {
     // copy children array to not loose cursor when children is spliced
     for (const child of node.children) {
-      visit(child, visitor);
+      visit(child, visitor, node);
     }
   }
   // visit element children if still attached to parent
   if (node.type === 'element') {
-    if (node.parentNode.children.includes(node)) {
+    if (parentNode.children.includes(node)) {
       for (const child of node.children) {
-        visit(child, visitor);
+        visit(child, visitor, node);
       }
     }
   }
   if (callbacks && callbacks.exit) {
-    callbacks.exit(node);
+    callbacks.exit(node, parentNode);
   }
 };
 exports.visit = visit;
 
-const detachNodeFromParent = (node) => {
+const detachNodeFromParent = (node, parentNode) => {
-  const parentNode = node.parentNode;
   // avoid splice to not break for loops
   parentNode.children = parentNode.children.filter((child) => child !== node);
 };

lib/xast.test.js

@@ -95,10 +95,10 @@ describe('xast', () => {
     const entered = [];
     visit(root, {
       element: {
-        enter: (node) => {
+        enter: (node, parentNode) => {
           entered.push(node.name);
           if (node.name === 'g') {
-            detachNodeFromParent(node);
+            detachNodeFromParent(node, parentNode);
           }
         },
       },

plugins/mergePaths.js

@@ -88,7 +88,7 @@ exports.fn = (root, params) => {
               floatPrecision,
               noSpaceAfterFlags,
             });
-            detachNodeFromParent(child);
+            detachNodeFromParent(child, node);
             continue;
           }
 

plugins/mergeStyles.js

@@ -17,7 +17,7 @@ exports.fn = () => {
   let collectedStyles = '';
   let styleContentType = 'text';
 
-  const enterElement = (node) => {
+  const enterElement = (node, parentNode) => {
     // collect style elements
     if (node.name !== 'style') {
       return;
@@ -51,7 +51,7 @@ exports.fn = () => {
 
     // remove empty style elements
     if (css.trim().length === 0) {
-      detachNodeFromParent(node);
+      detachNodeFromParent(node, parentNode);
       return;
     }
 
@@ -67,7 +67,7 @@ exports.fn = () => {
     if (firstStyleElement == null) {
       firstStyleElement = node;
     } else {
-      detachNodeFromParent(node);
+      detachNodeFromParent(node, parentNode);
       firstStyleElement.children = [
         new JSAPI(
           { type: styleContentType, value: collectedStyles },

plugins/removeComments.js

@@ -1,9 +1,9 @@
 'use strict';
 
-exports.type = 'perItem';
+const { detachNodeFromParent } = require('../lib/xast.js');
 
+exports.type = 'visitor';
 exports.active = true;
-
 exports.description = 'removes comments';
 
 /**
@@ -13,13 +13,16 @@ exports.description = 'removes comments';
  * <!-- Generator: Adobe Illustrator 15.0.0, SVG Export
  * Plug-In . SVG Version: 6.00 Build 0)  -->
  *
- * @param {Object} item current iteration item
- * @return {Boolean} if false, item will be filtered out
- *
  * @author Kir Belevich
  */
-exports.fn = function (item) {
+exports.fn = () => {
-  if (item.type === 'comment' && item.value.charAt(0) !== '!') {
+  return {
-    return false;
+    comment: {
-  }
+      enter: (node, parentNode) => {
+        if (node.value.charAt(0) !== '!') {
+          detachNodeFromParent(node, parentNode);
+        }
+      },
+    },
+  };
 };

plugins/removeHiddenElems.js

@@ -53,7 +53,7 @@ exports.fn = (root, params) => {
 
   return {
     element: {
-      enter: (node) => {
+      enter: (node, parentNode) => {
         // Removes hidden elements
         // https://www.w3schools.com/cssref/pr_class_visibility.asp
         const computedStyle = computeStyle(stylesheet, node);
@@ -65,7 +65,7 @@ exports.fn = (root, params) => {
           // keep if any descendant enables visibility
           querySelector(node, '[visibility=visible]') == null
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -82,7 +82,7 @@ exports.fn = (root, params) => {
           // markers with display: none still rendered
           node.name !== 'marker'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -97,7 +97,7 @@ exports.fn = (root, params) => {
           // transparent element inside clipPath still affect clipped elements
           closestByName(node, 'clipPath') == null
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -113,7 +113,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.r === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -129,7 +129,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.rx === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -145,7 +145,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.ry === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -161,7 +161,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.width === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -178,7 +178,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.height === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -193,7 +193,7 @@ exports.fn = (root, params) => {
           node.name === 'pattern' &&
           node.attributes.width === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -208,7 +208,7 @@ exports.fn = (root, params) => {
           node.name === 'pattern' &&
           node.attributes.height === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -223,7 +223,7 @@ exports.fn = (root, params) => {
           node.name === 'image' &&
           node.attributes.width === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -238,7 +238,7 @@ exports.fn = (root, params) => {
           node.name === 'image' &&
           node.attributes.height === '0'
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -249,12 +249,12 @@ exports.fn = (root, params) => {
         // <path d=""/>
         if (pathEmptyD && node.name === 'path') {
           if (node.attributes.d == null) {
-            detachNodeFromParent(node);
+            detachNodeFromParent(node, parentNode);
             return;
           }
           const pathData = parsePathData(node.attributes.d);
           if (pathData.length === 0) {
-            detachNodeFromParent(node);
+            detachNodeFromParent(node, parentNode);
             return;
           }
           // keep single point paths for markers
@@ -263,7 +263,7 @@ exports.fn = (root, params) => {
             computedStyle['marker-start'] == null &&
             computedStyle['marker-end'] == null
           ) {
-            detachNodeFromParent(node);
+            detachNodeFromParent(node, parentNode);
             return;
           }
           return;
@@ -279,7 +279,7 @@ exports.fn = (root, params) => {
           node.name === 'polyline' &&
           node.attributes.points == null
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
 
@@ -293,7 +293,7 @@ exports.fn = (root, params) => {
           node.name === 'polygon' &&
           node.attributes.points == null
         ) {
-          detachNodeFromParent(node);
+          detachNodeFromParent(node, parentNode);
           return;
         }
       },