finance: ensure that user data isn't mutated in Plots.supplyDefaults

  for ohlc and candlestick traces

Author: etpinard

src/plots/plots.js

@@ -805,7 +805,7 @@ function supplyTransformDefaults(traceIn, traceOut, layout) {
         if(!_module) Lib.warn('Unrecognized transform type ' + type + '.');
 
         if(_module && _module.supplyDefaults) {
-            transformOut = _module.supplyDefaults(transformIn, traceOut, layout);
+            transformOut = _module.supplyDefaults(transformIn, traceOut, layout, traceIn);
             transformOut.type = type;
         }
         else {

src/traces/candlestick/transform.js

@@ -18,7 +18,8 @@ exports.name = 'candlestick';
 
 exports.attributes = {};
 
-exports.supplyDefaults = function(transformIn, traceOut) {
+exports.supplyDefaults = function(transformIn, traceOut, layout, traceIn) {
+    helpers.clearEphemeralTransformOpts(traceIn);
     helpers.copyOHLC(transformIn, traceOut);
 
     return transformIn;

src/traces/ohlc/helpers.js

@@ -19,7 +19,8 @@ var Lib = require('../../lib');
 // Note that, we must mutate user data (here traceIn) as opposed
 // to full data (here traceOut) as - at the moment - transform
 // defaults (which are called after trace defaults) start
-// from a clear transforms container.
+// from a clear transforms container. The mutations inflicted are
+// cleared in exports.clearEphemeralTransformOpts.
 exports.pushDummyTransformOpts = function(traceIn, traceOut) {
     var transformOpts = {
 
@@ -31,29 +32,36 @@ exports.pushDummyTransformOpts = function(traceIn, traceOut) {
     };
 
     if(Array.isArray(traceIn.transforms)) {
-        var transformsIn = traceIn.transforms;
-
-        // remove all ephemeral transforms,
-        // before adding dummy transform
-        for(var i = 0; i < transformsIn.length; i++) {
-            if(transformsIn[i]._ephemeral) transformsIn.splice(i, 1);
-        }
-
         traceIn.transforms.push(transformOpts);
     }
     else {
         traceIn.transforms = [transformOpts];
     }
 };
 
-// This routine gets called during the transform supply-defaults step,
-// but only has an effect (because of the if-statements) during the
-// second round of transform defaults done on generated traces.
-//
-// This is a hacky way to pass 'ohlc' and 'candlestick' attributes
+// This routine gets called during the transform supply-defaults step
+// where it clears ephemeral transform opts in user data
+// and effectively put back user date in its pre-supplyDefaults state.
+exports.clearEphemeralTransformOpts = function(traceIn) {
+    var transformsIn = traceIn.transforms;
+
+    if(!Array.isArray(transformsIn)) return;
+
+    for(var i = 0; i < transformsIn.length; i++) {
+        if(transformsIn[i]._ephemeral) transformsIn.splice(i, 1);
+    }
+
+    if(transformsIn.length === 0) delete traceIn.transforms;
+};
+
+// This routine gets called during the transform supply-defaults step
+// where it passes 'ohlc' and 'candlestick' attributes
 // (found the transform container via exports.makeTransform)
-// to the traceOut container such that they can be compatible with
-// filter and groupby transforms.
+// to the traceOut container such that they can
+// be compatible with filter and groupby transforms.
+//
+// Note that this routine only has an effect during the
+// second round of transform defaults done on generated traces
 exports.copyOHLC = function(container, traceOut) {
     if(container.open) traceOut.open = container.open;
     if(container.high) traceOut.high = container.high;

src/traces/ohlc/transform.js

@@ -20,7 +20,8 @@ exports.name = 'ohlc';
 
 exports.attributes = {};
 
-exports.supplyDefaults = function(transformIn, traceOut) {
+exports.supplyDefaults = function(transformIn, traceOut, layout, traceIn) {
+    helpers.clearEphemeralTransformOpts(traceIn);
     helpers.copyOHLC(transformIn, traceOut);
 
     return transformIn;

test/jasmine/tests/finance_test.js

@@ -55,7 +55,7 @@ describe('finance charts defaults:', function() {
         expect(directions).toEqual(['increasing', 'decreasing', 'increasing', 'decreasing']);
     });
 
-    it('unfortunately mutates user trace', function() {
+    it('should not mutate user data', function() {
         var trace0 = Lib.extendDeep({}, mock0, {
             type: 'ohlc'
         });
@@ -65,18 +65,18 @@ describe('finance charts defaults:', function() {
         });
 
         var out = _supply([trace0, trace1]);
-        expect(out.data[0].transforms.length).toEqual(1);
-        expect(out.data[0].transforms).toEqual([{ type: 'ohlc', _ephemeral: true }]);
-        expect(out.data[1].transforms.length).toEqual(1);
-        expect(out.data[1].transforms).toEqual([{ type: 'candlestick', _ephemeral: true }]);
+        expect(out.data[0]).toBe(trace0);
+        expect(out.data[0].transforms).toBeUndefined();
+        expect(out.data[1]).toBe(trace1);
+        expect(out.data[1].transforms).toBeUndefined();
 
-        // but at least in an idempotent way
+        // ... and in an idempotent way
 
         var out2 = _supply(out.data);
-        expect(out2.data[0].transforms.length).toEqual(1);
-        expect(out2.data[0].transforms).toEqual([{ type: 'ohlc', _ephemeral: true }]);
-        expect(out2.data[0].transforms.length).toEqual(1);
-        expect(out2.data[1].transforms).toEqual([{ type: 'candlestick', _ephemeral: true }]);
+        expect(out2.data[0]).toBe(trace0);
+        expect(out2.data[0].transforms).toBeUndefined();
+        expect(out2.data[1]).toBe(trace1);
+        expect(out2.data[1].transforms).toBeUndefined();
     });
 
     it('should work with transforms', function() {
@@ -99,7 +99,15 @@ describe('finance charts defaults:', function() {
         expect(out.data.length).toEqual(2);
         expect(out._fullData.length).toEqual(4);
 
-        var transformTypes = out._fullData.map(function(fullTrace) {
+        var transformTypesIn = out.data.map(function(trace) {
+            return trace.transforms.map(function(opts) {
+                return opts.type;
+            });
+        });
+
+        expect(transformTypesIn).toEqual([ ['filter'], ['filter'] ]);
+
+        var transformTypesOut = out._fullData.map(function(fullTrace) {
             return fullTrace.transforms.map(function(opts) {
                 return opts.type;
             });
@@ -108,7 +116,7 @@ describe('finance charts defaults:', function() {
         // dummy 'ohlc' and 'candlestick' transforms are pushed at the end
         // of the 'transforms' array container
 
-        expect(transformTypes).toEqual([
+        expect(transformTypesOut).toEqual([
             ['filter', 'ohlc'], ['filter', 'ohlc'],
             ['filter', 'candlestick'], ['filter', 'candlestick']
         ]);