Internal state (options) can be modified asynchronously

[eliseevmikhail]

xmlToJSON uses single instance for every call, and every call modifies options object at parseXML function. Is it better to create onetime local copy of options for every call? Like this:

--- xmlToJSON.js-orig   2016-08-17 09:50:51.310936170 +0700
+++ xmlToJSON.js        2016-08-17 09:59:52.543327593 +0700
@@ -57,14 +57,16 @@
     };

     this.parseString = function(xmlString, opt) {
-        return this.parseXML(this.stringToXML(xmlString), opt);
+        var localoptions = {}
+        opt = opt || {}
+            // initialize options
+        for (var key in options) {
+            localoptions[key] = (opt[key] === undefined) ? options[key] : opt[key];
+        }
+        return this.parseXML(this.stringToXML(xmlString), localoptions);
     }

-    this.parseXML = function(oXMLParent, opt) {
-        // initialize options
-        for (var key in opt) {
-            options[key] = opt[key];
-        }
+    this.parseXML = function(oXMLParent, options) {
         var vResult = {},
             nLength = 0,
             sCollectedTxt = "";
@@ -154,7 +156,7 @@
                         sProp = oNode.nodeName;
                     }

-                    vContent = xmlToJSON.parseXML(oNode);
+                    vContent = xmlToJSON.parseXML(oNode, options);
                     if (!vResult['_children']) vResult._children = [];
                     vResult._children.push(vContent);
                     vContent._tagname = sProp;

[ghost]

I agree. Push this and I'll test it, update the version, and kick a new
release sometime next week (currently traveling)

On Aug 16, 2016 8:01 PM, "eliseevmikhail" notifications@github.com wrote:

xmlToJSON uses single instance for every call, and every call modifies
options object at parseXML function. Is it better to create onetime local
copy of options for every call? Like this:

--- xmlToJSON.js-orig 2016-08-17 09:50:51.310936170 +0700
+++ xmlToJSON.js 2016-08-17 09:59:52.543327593 +0700
@@ -57,14 +57,16 @@
};

 this.parseString = function(xmlString, opt) {

•    return this.parseXML(this.stringToXML(xmlString), opt);
  

•    var localoptions = {}
  

•    opt = opt || {}
  

•        // initialize options
  

•    for (var key in options) {
  

•        localoptions[key] = (opt[key] === undefined) ? options[key] : opt[key];
  

•    }
  

•    return this.parseXML(this.stringToXML(xmlString), localoptions);
  

  }

• this.parseXML = function(oXMLParent, opt) {

•    // initialize options
  

•    for (var key in opt) {
  

•        options[key] = opt[key];
  

•    }
  

• this.parseXML = function(oXMLParent, options) {
  var vResult = {},
  nLength = 0,
  sCollectedTxt = "";
  @@ -154,7 +156,7 @@
  sProp = oNode.nodeName;
  }

•                vContent = xmlToJSON.parseXML(oNode);
  

•                vContent = xmlToJSON.parseXML(oNode, options);
               if (!vResult['_children']) vResult._children = [];
               vResult._children.push(vContent);
               vContent._tagname = sProp;
  

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#25, or mute the thread
https://github.com/notifications/unsubscribe-auth/AFAjfDCdM944P_xjbDmo9m0VrB757n74ks5qgnmGgaJpZM4JmDA7
.

[alec-ferguson-sunrun]

I noticed this as well. It looks like subsequent calls to parseString() preserve the options from the previous call.

> xml = "<ClockCyclesGalore>CPU's working overtime</ClockCyclesGalore>"
'<ClockCyclesGalore>CPU\'s working overtime</ClockCyclesGalore>'
> parsed1 = xmlToJSON.parseString(xml, { textKey: 'CustomTextKey' })
{ ClockCyclesGalore: [ { CustomTextKey: 'CPU\'s working overtime' } ] }
> parsed2 = xmlToJSON.parseString(xml)
{ ClockCyclesGalore: [ { ShouldBeDifferentKey: 'CPU\'s working overtime' } ] }
> parsed1 = xmlToJSON.parseString(xml, { textKey: 'CustomTextKey' })
{ ClockCyclesGalore: [ { CustomTextKey: 'CPU\'s working overtime' } ] }
> parsed2 = xmlToJSON.parseString(xml)
{ ClockCyclesGalore: [ { CustomTextKey: 'CPU\'s working overtime' } ] }

On second call (parsed2) I would expect the textKey to be the default _text. However, since options is modified it is set for subsequent calls.

I recommend:

1. Define options outside of the core function
2. In function usage, use Object.assign to deep copy the options object.

I'm happy to submit a PR if you'd like.