close #1410 - yes, stop pruning in nestedProperty

Author: alexcjohnson

src/lib/nested_property.js

@@ -11,8 +11,6 @@
 
 var isNumeric = require('fast-isnumeric');
 var isArrayOrTypedArray = require('./is_array').isArrayOrTypedArray;
-var isPlainObject = require('./is_plain_object');
-var containerArrayMatch = require('../plot_api/container_array_match');
 
 /**
  * convert a string s (such as 'xaxis.range[0]')
@@ -115,44 +113,21 @@ function npGet(cont, parts) {
 }
 
 /*
- * Can this value be deleted? We can delete any empty object (null, undefined, [], {})
- * EXCEPT empty data arrays, {} inside an array, or anything INSIDE an *args* array.
+ * Can this value be deleted? We can delete `undefined`, and `null` except INSIDE an
+ * *args* array.
  *
- * Info arrays can be safely deleted, but not deleting them has no ill effects other
- * than leaving a trace or layout object with some cruft in it.
+ * Previously we also deleted some `{}` and `[]`, in order to try and make set/unset
+ * a net noop; but this causes far more complication than it's worth, and still had
+ * lots of exceptions. See https://github.com/plotly/plotly.js/issues/1410
  *
- * Deleting data arrays can change the meaning of the object, as `[]` means there is
- * data for this attribute, it's just empty right now while `undefined` means the data
- * should be filled in with defaults to match other data arrays.
- *
- * `{}` inside an array means "the default object" which is clearly different from
- * popping it off the end of the array, or setting it `undefined` inside the array.
- *
- * *args* arrays get passed directly to API methods and we should respect precisely
- * what the user has put there - although if the whole *args* array is empty it's fine
- * to delete that.
- *
- * So we do some simple tests here to find known non-data arrays but don't worry too
- * much about not deleting some arrays that would actually be safe to delete.
+ * *args* arrays get passed directly to API methods and we should respect null if
+ * the user put it there, but otherwise null is deleted as we use it as code
+ * in restyle/relayout/update for "delete this value" whereas undefined means
+ * "ignore this edit"
  */
-var INFO_PATTERNS = /(^|\.)((domain|range)(\.[xy])?|args|parallels)$/;
 var ARGS_PATTERN = /(^|\.)args\[/;
 function isDeletable(val, propStr) {
-    if(!emptyObj(val) ||
-        (isPlainObject(val) && propStr.charAt(propStr.length - 1) === ']') ||
-        (propStr.match(ARGS_PATTERN) && val !== undefined)
-    ) {
-        return false;
-    }
-    if(!isArrayOrTypedArray(val)) return true;
-
-    if(propStr.match(INFO_PATTERNS)) return true;
-
-    var match = containerArrayMatch(propStr);
-    // if propStr matches the container array itself, index is an empty string
-    // otherwise we've matched something inside the container array, which may
-    // still be a data array.
-    return match && (match.index === '');
+    return (val === undefined) || (val === null && !propStr.match(ARGS_PATTERN));
 }
 
 function npSet(cont, parts, propStr) {
@@ -194,8 +169,18 @@ function npSet(cont, parts, propStr) {
         }
 
         if(toDelete) {
-            if(i === parts.length - 1) delete curCont[parts[i]];
-            pruneContainers(containerLevels);
+            if(i === parts.length - 1) {
+                delete curCont[parts[i]];
+
+                // The one bit of pruning we still do: drop `undefined` from the end of arrays.
+                // In case someone has already unset previous items, continue until we hit a
+                // non-undefined value.
+                if(Array.isArray(curCont) && +parts[i] === curCont.length - 1) {
+                    while(curCont.length && curCont[curCont.length - 1] === undefined) {
+                        curCont.pop();
+                    }
+                }
+            }
         }
         else curCont[parts[i]] = val;
     };
@@ -249,48 +234,6 @@ function checkNewContainer(container, part, nextPart, toDelete) {
     return true;
 }
 
-function pruneContainers(containerLevels) {
-    var i,
-        j,
-        curCont,
-        propPart,
-        keys,
-        remainingKeys;
-    for(i = containerLevels.length - 1; i >= 0; i--) {
-        curCont = containerLevels[i][0];
-        propPart = containerLevels[i][1];
-
-        remainingKeys = false;
-        if(isArrayOrTypedArray(curCont)) {
-            for(j = curCont.length - 1; j >= 0; j--) {
-                if(isDeletable(curCont[j], joinPropStr(propPart, j))) {
-                    if(remainingKeys) curCont[j] = undefined;
-                    else curCont.pop();
-                }
-                else remainingKeys = true;
-            }
-        }
-        else if(typeof curCont === 'object' && curCont !== null) {
-            keys = Object.keys(curCont);
-            remainingKeys = false;
-            for(j = keys.length - 1; j >= 0; j--) {
-                if(isDeletable(curCont[keys[j]], joinPropStr(propPart, keys[j]))) {
-                    delete curCont[keys[j]];
-                }
-                else remainingKeys = true;
-            }
-        }
-        if(remainingKeys) return;
-    }
-}
-
-function emptyObj(obj) {
-    if(obj === undefined || obj === null) return true;
-    if(typeof obj !== 'object') return false; // any plain value
-    if(isArrayOrTypedArray(obj)) return !obj.length; // []
-    return !Object.keys(obj).length; // {}
-}
-
 function badContainer(container, propStr, propParts) {
     return {
         set: function() { throw 'bad container'; },

test/jasmine/tests/legend_test.js

@@ -973,8 +973,8 @@ describe('legend interaction', function() {
             }).then(function() {
                 // Verify the group names have been cleaned up:
                 expect(gd.data[1].transforms[0].styles).toEqual([
-                    {target: 3},
-                    {target: 4}
+                    {target: 3, value: {}},
+                    {target: 4, value: {}}
                 ]);
             }).catch(fail).then(done);
         });

test/jasmine/tests/lib_test.js

@@ -328,7 +328,7 @@ describe('Test lib.js:', function() {
             expect(obj).toEqual({a: false, b: '', c: 0, d: NaN});
         });
 
-        it('should not remove data arrays or empty objects inside container arrays', function() {
+        it('should not remove arrays or empty objects inside container arrays', function() {
             var obj = {
                     annotations: [{a: [1, 2, 3]}],
                     c: [1, 2, 3],
@@ -351,11 +351,17 @@ describe('Test lib.js:', function() {
             propS.set(null);
 
             // 'a' and 'c' are both potentially data arrays so we need to keep them
-            expect(obj).toEqual({annotations: [{a: []}], c: []});
+            expect(obj).toEqual({
+                annotations: [{a: []}],
+                c: [],
+                domain: [],
+                range: [],
+                shapes: []
+            });
         });
 
 
-        it('should allow empty object sub-containers only in arrays', function() {
+        it('should allow empty object sub-containers', function() {
             var obj = {},
                 prop = np(obj, 'a[1].b.c'),
                 // we never set a value into a[0] so it doesn't even get {}
@@ -370,15 +376,15 @@ describe('Test lib.js:', function() {
 
             prop.set(null);
             expect(prop.get()).toBe(undefined);
-            expect(obj).toEqual({a: [undefined, {}]});
+            expect(obj).toEqual({a: [undefined, {b: {}}]});
         });
 
         it('does not prune inside `args` arrays', function() {
             var obj = {},
                 args = np(obj, 'args');
 
             args.set([]);
-            expect(obj.args).toBeUndefined();
+            expect(obj.args).toEqual([]);
 
             args.set([null]);
             expect(obj.args).toEqual([null]);
@@ -1950,7 +1956,7 @@ describe('Test lib.js:', function() {
 
                 expect(container).toEqual({styles: [
                     {foo: 'name4', bar: {value: 'value1'}},
-                    {foo: 'name2'},
+                    {foo: 'name2', bar: {}},
                     {foo: 'name3', bar: {value: 'value3'}}
                 ]});
 
@@ -1971,7 +1977,7 @@ describe('Test lib.js:', function() {
 
                 carr.remove('name');
 
-                expect(container.styles).toEqual([{foo: 'name', extra: 'data'}]);
+                expect(container.styles).toEqual([{foo: 'name', bar: {}, extra: 'data'}]);
 
                 expect(carr.constructUpdate()).toEqual({
                     'styles[0].bar.value': null,
@@ -2006,7 +2012,7 @@ describe('Test lib.js:', function() {
 
                 expect(container.styles).toEqual([
                     {foo: 'name1', bar: {extra: 'data'}},
-                    {foo: 'name2'},
+                    {foo: 'name2', bar: {}},
                     {foo: 'name3', bar: {value: 'value3', extra: 'data'}},
                 ]);
 
@@ -2029,7 +2035,7 @@ describe('Test lib.js:', function() {
                 carr.remove('name1');
 
                 expect(container.styles).toEqual([
-                    {foo: 'name1'},
+                    {foo: 'name1', bar: {}},
                     {foo: 'name2', bar: {value: 'value2', extra: 'data2'}},
                 ]);
 

test/jasmine/tests/sliders_test.js

@@ -100,17 +100,20 @@ describe('sliders defaults', function() {
             method: 'relayout',
             label: 'Label #1',
             value: 'label-1',
-            execute: true
+            execute: true,
+            args: []
         }, {
             method: 'update',
             label: 'Label #2',
             value: 'Label #2',
-            execute: true
+            execute: true,
+            args: []
         }, {
             method: 'animate',
             label: 'step-2',
             value: 'lacks-label',
-            execute: true
+            execute: true,
+            args: []
         }]);
     });