Fix keyedContainer behavior to unset entries without removing

plotly · rreusser · Aug 15, 2017 · Jul 27, 2017 · Jul 27, 2017 · Jul 27, 2017
commit 4943e3a4771bb7b9e1698c7a17e1e34d3ecfce1c
diff --git a/src/lib/keyed_container.js b/src/lib/keyed_container.js
@@ -12,7 +12,18 @@ var nestedProperty = require('./nested_property');
 
 var SIMPLE_PROPERTY_REGEX = /^\w*$/;
 
-// bitmask for deciding what's updated:
+// bitmask for deciding what's updated. Sometimes the name needs to be updated,
+// sometimes the value needs to be updated, and sometimes both do. This is just
+// a simple way to track what's updated such that it's a simple OR operation to
+// assimilate new updates.
+//
+// The only exception is the UNSET bit that tracks when we need to explicitly
+// unset and remove the property. This concrn arises because of the special
+// way in which nestedProperty handles null/undefined. When you specify `null`,
+// it prunes any unused items in the tree. I ran into some issues with it getting
+// null vs undefined confused, so UNSET is just a bit that forces the property
+// update to send `null`, removing the property explicitly rather than setting
+// it to undefined.
 var NONE = 0;
 var NAME = 1;
 var VALUE = 2;
@@ -111,14 +122,27 @@ module.exports = function keyedContainer(baseObj, path, keyName, valueName) {
                 return obj.set(name, null);
             }
 
-            for(i = idx; i < arr.length; i++) {
-                changeTypes[i] = changeTypes[i] | BOTH;
-            }
-            for(i = idx; i < arr.length; i++) {
-                indexLookup[arr[i][keyName]]--;
+            if(isSimpleValueProp) {
+                for(i = idx; i < arr.length; i++) {
+                    changeTypes[i] = changeTypes[i] | BOTH;
+                }
+                for(i = idx; i < arr.length; i++) {
+                    indexLookup[arr[i][keyName]]--;
+                }
+                arr.splice(idx, 1);
+                delete(indexLookup[name]);
+            } else {
+                // Perform this update *strictly* so we can check whether the result's
+                // been pruned. If so, it's a removal. If not, it's a value unset only.
+                nestedProperty(object, valueName).set(null);
+
+                // Now check if the top level nested property has any keys left. If so,
+                // the object still has values so we only want to unset the key. If not,
+                // the entire object can be removed since there's no other data.
+                // var topLevelKeys = Object.keys(object[valueName.split('.')[0]] || []);
+
+                changeTypes[idx] = changeTypes[idx] | VALUE | UNSET;
             }
-            arr.splice(idx, 1);
-            delete(indexLookup[name]);
 
             return obj;
         },

diff --git a/test/jasmine/tests/legend_test.js b/test/jasmine/tests/legend_test.js
@@ -945,7 +945,10 @@ describe('legend interaction', function() {
                 return _setValue(4, '');
             }).then(function() {
                 // Verify the group names have been cleaned up:
-                expect(gd.data[1].transforms[0].styles).toEqual([]);
+                expect(gd.data[1].transforms[0].styles).toEqual([
+                    {target: 3},
+                    {target: 4}
+                ]);
             }).catch(fail).then(done);
         });
     });

diff --git a/test/jasmine/tests/lib_test.js b/test/jasmine/tests/lib_test.js
@@ -1817,18 +1817,19 @@ describe('Test lib.js:', function() {
 
                 carr.set('name3', 'value3');
                 carr.remove('name2');
-                carr.rename('name1', 'name2');
+                carr.rename('name1', 'name4');
 
                 expect(container).toEqual({styles: [
-                    {foo: 'name2', bar: {value: 'value1'}},
+                    {foo: 'name4', bar: {value: 'value1'}},
+                    {foo: 'name2'},
                     {foo: 'name3', bar: {value: 'value3'}}
                 ]});
 
                 expect(carr.constructUpdate()).toEqual({
-                    'styles[0].foo': 'name2',
-                    'styles[1].foo': 'name3',
-                    'styles[1].bar.value': 'value3',
-                    'styles[2]': null
+                    'styles[0].foo': 'name4',
+                    'styles[1].bar.value': null,
+                    'styles[2].foo': 'name3',
+                    'styles[2].bar.value': 'value3',
                 });
             });
 
@@ -1847,6 +1848,66 @@ describe('Test lib.js:', function() {
                     'styles[0].bar.value': null,
                 });
             });
+
+            it('unsets but does not remove items with extra value data', function() {
+                var container = {styles: [
+                    {foo: 'name1', bar: {value: 'value1', extra: 'data'}},
+                    {foo: 'name2', bar: {value: 'value2'}},
+                    {foo: 'name3', bar: {value: 'value3', extra: 'data'}},
+                ]};
+
+                var carr = Lib.keyedContainer(container, 'styles', 'foo', 'bar.value');
+
+                // Remove the first value:
+
+                carr.remove('name1');
+
+                expect(container.styles).toEqual([
+                    {foo: 'name1', bar: {extra: 'data'}},
+                    {foo: 'name2', bar: {value: 'value2'}},
+                    {foo: 'name3', bar: {value: 'value3', extra: 'data'}},
+                ]);
+
+                expect(carr.constructUpdate()).toEqual({
+                    'styles[0].bar.value': null
+                });
+
+                // Remove the second value:
+                carr.remove('name2');
+
+                expect(container.styles).toEqual([
+                    {foo: 'name1', bar: {extra: 'data'}},
+                    {foo: 'name2'},
+                    {foo: 'name3', bar: {value: 'value3', extra: 'data'}},
+                ]);
+
+                expect(carr.constructUpdate()).toEqual({
+                    'styles[0].bar.value': null,
+                    'styles[1].bar.value': null
+                });
+            });
+
+            it('does not compress nested attributes *sigh*', function() {
+                var container = {styles: [
+                    {foo: 'name1', bar: {value: 'value1'}},
+                    {foo: 'name2', bar: {value: 'value2', extra: 'data2'}},
+                ]};
+
+                var carr = Lib.keyedContainer(container, 'styles', 'foo', 'bar.value');
+
+                // Remove the first value:
+
+                carr.remove('name1');
+
+                expect(container.styles).toEqual([
+                    {foo: 'name1'},
+                    {foo: 'name2', bar: {value: 'value2', extra: 'data2'}},
+                ]);
+
+                expect(carr.constructUpdate()).toEqual({
+                    'styles[0].bar.value': null
+                });
+            });
         });
     });