react+transforms PR review edits

- is1D -> isArray1D
- improved comments
- opts object instead of confusing boolean to Plots.supplyDefaults
- merge & simplify remapTransformedArrays
- hasX

Author: alexcjohnson

Diff for: src/lib/index.js

@@ -30,7 +30,7 @@ lib.ensureArray = require('./ensure_array');
 var isArrayModule = require('./is_array');
 lib.isTypedArray = isArrayModule.isTypedArray;
 lib.isArrayOrTypedArray = isArrayModule.isArrayOrTypedArray;
-lib.is1D = isArrayModule.is1D;
+lib.isArray1D = isArrayModule.isArray1D;
 
 var coerceModule = require('./coerce');
 lib.valObjectMeta = coerceModule.valObjectMeta;

Diff for: src/lib/is_array.js

@@ -26,12 +26,20 @@ function isArrayOrTypedArray(a) {
     return Array.isArray(a) || isTypedArray(a);
 }
 
-function is1D(a) {
+/*
+ * Test whether an input object is 1D.
+ *
+ * Assumes we already know the object is an array.
+ *
+ * Looks only at the first element, if the dimensionality is
+ * not consistent we won't figure that out here.
+ */
+function isArray1D(a) {
     return !isArrayOrTypedArray(a[0]);
 }
 
 module.exports = {
     isTypedArray: isTypedArray,
     isArrayOrTypedArray: isArrayOrTypedArray,
-    is1D: is1D
+    isArray1D: isArray1D
 };

Diff for: src/plot_api/plot_api.js

@@ -2270,7 +2270,7 @@ exports.react = function(gd, data, layout, config) {
         // "true" skips updating calcdata and remapping arrays from calcTransforms,
         // which supplyDefaults usually does at the end, but we may need to NOT do
         // if the diff (which we haven't determined yet) says we'll recalc
-        Plots.supplyDefaults(gd, true);
+        Plots.supplyDefaults(gd, {skipUpdateCalc: true});
 
         var newFullData = gd._fullData;
         var newFullLayout = gd._fullLayout;

Diff for: src/plots/plots.js

@@ -254,27 +254,37 @@ var extraFormatKeys = [
     'year', 'month', 'dayMonth', 'dayMonthYear'
 ];
 
-// Fill in default values:
-//
-// gd.data, gd.layout:
-//   are precisely what the user specified,
-//   these fields shouldn't be modified nor used directly
-//   after the supply defaults step.
-//
-// gd._fullData, gd._fullLayout:
-//   are complete descriptions of how to draw the plot,
-//   use these fields in all required computations.
-//
-// gd._fullLayout._modules
-//   is a list of all the trace modules required to draw the plot.
-//
-// gd._fullLayout._basePlotModules
-//   is a list of all the plot modules required to draw the plot.
-//
-// gd._fullLayout._transformModules
-//   is a list of all the transform modules invoked.
-//
-plots.supplyDefaults = function(gd, skipCalcUpdate) {
+/*
+ * Fill in default values
+ * @param {DOM element} gd
+ * @param {object} opts
+ * @param {boolean} opts.skipUpdateCalc: normally if the existing gd.calcdata looks
+ *   compatible with the new gd._fullData we finish by linking the new _fullData traces
+ *   to the old gd.calcdata, so it's correctly set if we're not going to recalc. But also,
+ *   if there are calcTransforms on the trace, we first remap data arrays from the old full
+ *   trace into the new one. Use skipUpdateCalc to defer this (needed by Plotly.react)
+ *
+ * gd.data, gd.layout:
+ *   are precisely what the user specified,
+ *   these fields shouldn't be modified nor used directly
+ *   after the supply defaults step.
+ *
+ * gd._fullData, gd._fullLayout:
+ *   are complete descriptions of how to draw the plot,
+ *   use these fields in all required computations.
+ *
+ * gd._fullLayout._modules
+ *   is a list of all the trace modules required to draw the plot.
+ *
+ * gd._fullLayout._basePlotModules
+ *   is a list of all the plot modules required to draw the plot.
+ *
+ * gd._fullLayout._transformModules
+ *   is a list of all the transform modules invoked.
+ *
+ */
+plots.supplyDefaults = function(gd, opts) {
+    var skipUpdateCalc = opts && opts.skipUpdateCalc;
     var oldFullLayout = gd._fullLayout || {};
 
     if(oldFullLayout._skipDefaults) {
@@ -458,7 +468,7 @@ plots.supplyDefaults = function(gd, skipCalcUpdate) {
     }
 
     // update object references in calcdata
-    if(!skipCalcUpdate && oldCalcdata.length === newFullData.length) {
+    if(!skipUpdateCalc && oldCalcdata.length === newFullData.length) {
         plots.supplyDefaultsUpdateCalc(oldCalcdata, newFullData);
     }
 
@@ -471,11 +481,18 @@ plots.supplyDefaultsUpdateCalc = function(oldCalcdata, newFullData) {
         var newTrace = newFullData[i];
         var cd0 = oldCalcdata[i][0];
         if(cd0 && cd0.trace) {
-            if(cd0.trace._hasCalcTransform) {
-                remapTransformedArrays(cd0, newTrace);
-            } else {
-                cd0.trace = newTrace;
+            var oldTrace = cd0.trace;
+            if(oldTrace._hasCalcTransform) {
+                var arrayAttrs = oldTrace._arrayAttrs;
+                var j, astr, oldArrayVal;
+
+                for(j = 0; j < arrayAttrs.length; j++) {
+                    astr = arrayAttrs[j];
+                    oldArrayVal = Lib.nestedProperty(oldTrace, astr).get().slice();
+                    Lib.nestedProperty(newTrace, astr).set(oldArrayVal);
+                }
             }
+            cd0.trace = newTrace;
         }
     }
 };
@@ -522,25 +539,6 @@ function emptySubplotLists() {
     return out;
 }
 
-function remapTransformedArrays(cd0, newTrace) {
-    var oldTrace = cd0.trace;
-    var arrayAttrs = oldTrace._arrayAttrs;
-    var transformedArrayHash = {};
-    var i, astr;
-
-    for(i = 0; i < arrayAttrs.length; i++) {
-        astr = arrayAttrs[i];
-        transformedArrayHash[astr] = Lib.nestedProperty(oldTrace, astr).get().slice();
-    }
-
-    cd0.trace = newTrace;
-
-    for(i = 0; i < arrayAttrs.length; i++) {
-        astr = arrayAttrs[i];
-        Lib.nestedProperty(cd0.trace, astr).set(transformedArrayHash[astr]);
-    }
-}
-
 /**
  * getFormatObj: use _context to get the format object from locale.
  * Used to get d3.locale argument object and extraFormat argument object

Diff for: src/traces/box/defaults.js

@@ -51,7 +51,7 @@ function handleSampleDefaults(traceIn, traceOut, coerce, layout) {
             coerce('x0');
             len = y.length;
         }
-    } else if(x && x.length) {
+    } else if(hasX) {
         defaultOrientation = 'h';
         coerce('y0');
         len = x.length;

Diff for: src/traces/carpet/calc.js

@@ -9,7 +9,7 @@
 'use strict';
 
 var Axes = require('../../plots/cartesian/axes');
-var is1D = require('../../lib').is1D;
+var isArray1D = require('../../lib').isArray1D;
 var cheaterBasis = require('./cheater_basis');
 var arrayMinmax = require('./array_minmax');
 var calcGridlines = require('./calc_gridlines');
@@ -29,8 +29,8 @@ module.exports = function calc(gd, trace) {
     var x = trace.x;
     var y = trace.y;
     var cols = [];
-    if(x && is1D(x)) cols.push('x');
-    if(y && is1D(y)) cols.push('y');
+    if(x && isArray1D(x)) cols.push('x');
+    if(y && isArray1D(y)) cols.push('y');
 
     if(cols.length) {
         convertColumnData(trace, aax, bax, 'a', 'b', cols);

Diff for: src/traces/carpet/xy_defaults.js

@@ -9,7 +9,7 @@
 
 'use strict';
 
-var is1D = require('../../lib').is1D;
+var isArray1D = require('../../lib').isArray1D;
 
 module.exports = function handleXYDefaults(traceIn, traceOut, coerce) {
     var x = coerce('x');
@@ -20,7 +20,7 @@ module.exports = function handleXYDefaults(traceIn, traceOut, coerce) {
 
     traceOut._cheater = !x;
 
-    if((!hasX || is1D(x)) && (!hasY || is1D(y))) {
+    if((!hasX || isArray1D(x)) && (!hasY || isArray1D(y))) {
         var len = hasX ? x.length : Infinity;
         if(hasY) len = Math.min(len, y.length);
         if(traceOut.a && traceOut.a.length) len = Math.min(len, traceOut.a.length);

Diff for: src/traces/contour/defaults.js

@@ -35,7 +35,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
 
     coerce('text');
     var isConstraint = (coerce('contours.type') === 'constraint');
-    coerce('connectgaps', Lib.is1D(traceOut.z));
+    coerce('connectgaps', Lib.isArray1D(traceOut.z));
 
     // trace-level showlegend has already been set, but is only allowed if this is a constraint
     if(!isConstraint) delete traceOut.showlegend;

Diff for: src/traces/contourcarpet/calc.js

@@ -9,7 +9,7 @@
 'use strict';
 
 var colorscaleCalc = require('../../components/colorscale/calc');
-var is1D = require('../../lib').is1D;
+var isArray1D = require('../../lib').isArray1D;
 
 var convertColumnData = require('../heatmap/convert_column_xyz');
 var clean2dArray = require('../heatmap/clean_2d_array');
@@ -70,7 +70,7 @@ function heatmappishCalc(gd, trace) {
     aax._minDtick = 0;
     bax._minDtick = 0;
 
-    if(is1D(trace.z)) convertColumnData(trace, aax, bax, 'a', 'b', ['z']);
+    if(isArray1D(trace.z)) convertColumnData(trace, aax, bax, 'a', 'b', ['z']);
     a = trace._a = trace._a || trace.a;
     b = trace._b = trace._b || trace.b;
 

Diff for: src/traces/contourcarpet/defaults.js

@@ -56,7 +56,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
         var isConstraint = (coerce('contours.type') === 'constraint');
 
         // Unimplemented:
-        // coerce('connectgaps', Lib.is1D(traceOut.z));
+        // coerce('connectgaps', Lib.isArray1D(traceOut.z));
 
         // trace-level showlegend has already been set, but is only allowed if this is a constraint
         if(!isConstraint) delete traceOut.showlegend;

Diff for: src/traces/heatmap/calc.js

@@ -58,7 +58,7 @@ module.exports = function calc(gd, trace) {
     }
     else {
         var zIn = trace.z;
-        if(Lib.is1D(zIn)) {
+        if(Lib.isArray1D(zIn)) {
             convertColumnData(trace, xa, ya, 'x', 'y', ['z']);
             x = trace._x;
             y = trace._y;

Diff for: src/traces/heatmap/convert_column_xyz.js

@@ -17,7 +17,7 @@ module.exports = function convertColumnData(trace, ax1, ax2, var1Name, var2Name,
     var col1 = trace[var1Name].slice(0, colLen);
     var col2 = trace[var2Name].slice(0, colLen);
     var textCol = trace.text;
-    var hasColumnText = (textCol !== undefined && Lib.is1D(textCol));
+    var hasColumnText = (textCol !== undefined && Lib.isArray1D(textCol));
     var col1Calendar = trace[var1Name + 'calendar'];
     var col2Calendar = trace[var2Name + 'calendar'];
 

Diff for: src/traces/heatmap/defaults.js

@@ -32,7 +32,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
 
     handleStyleDefaults(traceIn, traceOut, coerce, layout);
 
-    coerce('connectgaps', Lib.is1D(traceOut.z) && (traceOut.zsmooth !== false));
+    coerce('connectgaps', Lib.isArray1D(traceOut.z) && (traceOut.zsmooth !== false));
 
     colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: '', cLetter: 'z'});
 };

Diff for: src/traces/heatmap/xyz_defaults.js

@@ -22,7 +22,7 @@ module.exports = function handleXYZDefaults(traceIn, traceOut, coerce, layout, x
 
     if(z === undefined || !z.length) return 0;
 
-    if(Lib.is1D(traceIn.z)) {
+    if(Lib.isArray1D(traceIn.z)) {
         x = coerce(xName);
         y = coerce(yName);
 

Diff for: src/traces/mesh3d/defaults.js

@@ -88,5 +88,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
     coerce('text');
 
     // disable 1D transforms
+    // x/y/z should match lengths, and i/j/k should match as well, but
+    // the two sets have different lengths so transforms wouldn't work.
     traceOut._length = null;
 };