Lock down and document the unimplemented case in expandObjectPaths

Author: rreusser

src/lib/index.js

@@ -589,9 +589,25 @@ lib.objectFromPath = function(path, value) {
 /**
  * Iterate through an object in-place, converting dotted properties to objects.
  *
- * @example
- * lib.expandObjectPaths({'nested.test.path': 'value'});
- * // returns { nested: { test: {path: 'value'}}}
+ * Examples:
+ *
+ *   lib.expandObjectPaths({'nested.test.path': 'value'});
+ *     => { nested: { test: {path: 'value'}}}
+ *
+ * It also handles array notation, e.g.:
+ *
+ *   lib.expandObjectPaths({'foo[1].bar': 'value'});
+ *     => { foo: [null, {bar: value}] }
+ *
+ * It handles merges the results when two properties are specified in parallel:
+ *
+ *   lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
+ *     => { foo: [{bar: 10}, {bar: 20}] }
+ *
+ * It does NOT, however, merge mulitple mutliply-nested arrays::
+ *
+ *   lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
+ *     => { marker: [null, {range: 4}] }
  */
 
 // Store this to avoid recompiling regex on *every* prop since this may happen many
@@ -624,10 +640,27 @@ lib.expandObjectPaths = function(data) {
                     data[prop] = data[prop] || [];
 
                     if(match[3] === '.') {
+                        // This is the case where theere are subsequent properties into which
+                        // we must recurse, e.g. transforms[0].value
                         trailingPath = match[4];
                         dest = data[prop][idx] = data[prop][idx] || {};
+
+                        // NB: Extend deep no arrays prevents this from working on multiple
+                        // nested properties in the same object, e.g.
+                        //
+                        // {
+                        //   foo[0].bar[1].range
+                        //   foo[0].bar[0].range
+                        // }
+                        //
+                        // In this case, the extendDeepNoArrays will overwrite one array with
+                        // the other, so that both properties *will not* be present in the
+                        // result. Fixing this would require a more intelligent tracking
+                        // of changes and merging than extendDeepNoArrays currently accomplishes.
                         lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
                     } else {
+                        // This is the case where this property is the end of the line,
+                        // e.g. xaxis.range[0]
                         data[prop][idx] = lib.expandObjectPaths(datum);
                     }
                 } else {

test/jasmine/tests/lib_test.js

@@ -579,12 +579,27 @@ describe('Test lib.js:', function() {
             expect(computed).toEqual(expected);
         });
 
+        it('combines changes with single array nesting', function() {
+            var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
+            var expected = {marker: [{foo: 4}, {foo: 5}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        // TODO: This test is unimplemented since it's a currently-unused corner case.
+        // Getting the test to pass requires some extension (pun?) to extendDeepNoArrays
+        // that's intelligent enough to only selectively merge *some* arrays, in particular
+        // not data arrays but yes on arrays that were previously expanded. This is a bit
+        // tricky to get to work just right and currently doesn't have any known use since
+        // container arrays are not multiply nested.
+        /*
         it('combines changes', function() {
             var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
             var expected = {marker: [undefined, {range: [4, 5]}]};
             var computed = Lib.expandObjectPaths(input);
             expect(computed).toEqual(expected);
         });
+        */
     });
 
     describe('coerce', function() {