Skip to content

Transform react #2577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Apr 26, 2018
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
bbe3533
remove no longer needed hack for finance in findArrayAttributes
alexcjohnson Apr 18, 2018
ed24765
_commonLength -> _length
alexcjohnson Apr 18, 2018
fcc459d
fix #2508, fix #2470 - problems with Plotly.react and aggregate trans…
alexcjohnson Apr 18, 2018
7044a13
standardize transforms handling of _length
alexcjohnson Apr 19, 2018
88b7b43
:racehorse: refactor sort transform from O(n^2) to O(n)
alexcjohnson Apr 19, 2018
cf4c9c3
heatmap&carpet/has_columns -> Lib.is1D
alexcjohnson Apr 20, 2018
e84d4b9
close #1410 - yes, stop pruning in nestedProperty
alexcjohnson Apr 20, 2018
b436d52
ensure every trace defines _length in supplyDefaults, and abort trans…
alexcjohnson Apr 22, 2018
2a41f9e
react+transforms PR review edits
alexcjohnson Apr 24, 2018
965bcfb
fixes and test for checklist in #2508
alexcjohnson Apr 24, 2018
6fae229
some fixes and tests for empty data arrays
alexcjohnson Apr 24, 2018
79295f1
rename violins mock that doesn't contain violin traces
alexcjohnson Apr 25, 2018
5e9aa65
transforms mock
alexcjohnson Apr 26, 2018
690eb95
clean up keyedContainer for possibly missing array
alexcjohnson Apr 26, 2018
a244cec
violin should not explicitly set whiskerwidth
alexcjohnson Apr 26, 2018
92bd5d2
add _length and stop slicing in scattercarpet
alexcjohnson Apr 26, 2018
dc6de2f
clean up handling of colorscale defaults
alexcjohnson Apr 26, 2018
03956e1
include count aggregates in _arrayAttrs - so they remap correctly
alexcjohnson Apr 26, 2018
f439e41
in diffData I had _fullInput in the new trace, but not the old!
alexcjohnson Apr 26, 2018
e47e6a9
_compareAsJSON for groupby styles
alexcjohnson Apr 26, 2018
aa30ad6
update plot_api_test to :lock: recent changes in react/transforms etc
alexcjohnson Apr 26, 2018
4c70826
lint
alexcjohnson Apr 26, 2018
7279b55
+shapes & annotations3d in react-noop test
alexcjohnson Apr 26, 2018
3e250df
tweak docs & remove commented-out code
alexcjohnson Apr 26, 2018
cd08479
reactWith -> reactTo
alexcjohnson Apr 26, 2018
0414147
separate svg and gl traces in react-noop tests
alexcjohnson Apr 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 49 additions & 24 deletions src/components/colorscale/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,47 @@ var flipScale = require('./flip_scale');


module.exports = function calc(trace, vals, containerStr, cLetter) {
var container, inputContainer;
var container = trace;
var inputContainer = trace._input;
var fullInputContainer = trace._fullInput;

// set by traces with groupby transforms
var updateStyle = trace.updateStyle;

function doUpdate(attr, inputVal, fullVal) {
if(fullVal === undefined) fullVal = inputVal;

if(updateStyle) {
updateStyle(trace._input, containerStr ? (containerStr + '.' + attr) : attr, inputVal);
}
else {
inputContainer[attr] = inputVal;
}

container[attr] = fullVal;
if(fullInputContainer && (trace !== trace._fullInput)) {
if(updateStyle) {
updateStyle(trace._fullInput, containerStr ? (containerStr + '.' + attr) : attr, fullVal);
}
else {
fullInputContainer[attr] = fullVal;
}
}
}

if(containerStr) {
container = Lib.nestedProperty(trace, containerStr).get();
inputContainer = Lib.nestedProperty(trace._input, containerStr).get();
}
else {
container = trace;
inputContainer = trace._input;
container = Lib.nestedProperty(container, containerStr).get();
inputContainer = Lib.nestedProperty(inputContainer, containerStr).get();
fullInputContainer = Lib.nestedProperty(fullInputContainer, containerStr).get() || {};
}

var autoAttr = cLetter + 'auto',
minAttr = cLetter + 'min',
maxAttr = cLetter + 'max',
auto = container[autoAttr],
min = container[minAttr],
max = container[maxAttr],
scl = container.colorscale;
var autoAttr = cLetter + 'auto';
var minAttr = cLetter + 'min';
var maxAttr = cLetter + 'max';
var auto = container[autoAttr];
var min = container[minAttr];
var max = container[maxAttr];
var scl = container.colorscale;

if(auto !== false || min === undefined) {
min = Lib.aggNums(Math.min, null, vals);
Expand All @@ -48,11 +71,8 @@ module.exports = function calc(trace, vals, containerStr, cLetter) {
max += 0.5;
}

container[minAttr] = min;
container[maxAttr] = max;

inputContainer[minAttr] = min;
inputContainer[maxAttr] = max;
doUpdate(minAttr, min);
doUpdate(maxAttr, max);

/*
* If auto was explicitly false but min or max was missing,
Expand All @@ -61,17 +81,22 @@ module.exports = function calc(trace, vals, containerStr, cLetter) {
* Otherwise make sure the trace still looks auto as far as later
* changes are concerned.
*/
inputContainer[autoAttr] = (auto !== false ||
(min === undefined && max === undefined));
doUpdate(autoAttr, (auto !== false || (min === undefined && max === undefined)));

if(container.autocolorscale) {
if(min * max < 0) scl = scales.RdBu;
else if(min >= 0) scl = scales.Reds;
else scl = scales.Blues;

// reversescale is handled at the containerOut level
inputContainer.colorscale = scl;
if(container.reversescale) scl = flipScale(scl);
container.colorscale = scl;
doUpdate('colorscale', scl, container.reversescale ? flipScale(scl) : scl);

// We pushed a colorscale back to input, which will change the default autocolorscale next time
// to avoid spurious redraws from Plotly.react, update resulting autocolorscale now
// This is a conscious decision so that changing the data later does not unexpectedly
// give you a new colorscale
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for 📚

if(!inputContainer.autocolorscale) {
doUpdate('autocolorscale', false);
}
}
};
1 change: 1 addition & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ lib.ensureArray = require('./ensure_array');
var isArrayModule = require('./is_array');
lib.isTypedArray = isArrayModule.isTypedArray;
lib.isArrayOrTypedArray = isArrayModule.isArrayOrTypedArray;
lib.isArray1D = isArrayModule.isArray1D;

var coerceModule = require('./coerce');
lib.valObjectMeta = coerceModule.valObjectMeta;
Expand Down
26 changes: 22 additions & 4 deletions src/lib/is_array.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,28 @@ var dv = (typeof DataView === 'undefined') ?
function() {} :
DataView;

exports.isTypedArray = function(a) {
function isTypedArray(a) {
return ab.isView(a) && !(a instanceof dv);
};
}

function isArrayOrTypedArray(a) {
return Array.isArray(a) || isTypedArray(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]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe isArray1D would be clearer as this assumes that you're passing an array (or at least something you can index in).

We might want to add a comment above about that assumption too, in case someone in the future wants to turn this into:

return Array.isArray(a) && !isArrayOrTypedArray(a[0]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2a41f9e -> isArray1D

}

exports.isArrayOrTypedArray = function(a) {
return Array.isArray(a) || exports.isTypedArray(a);
module.exports = {
isTypedArray: isTypedArray,
isArrayOrTypedArray: isArrayOrTypedArray,
isArray1D: isArray1D
};
26 changes: 20 additions & 6 deletions src/lib/keyed_container.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,32 +33,44 @@ var UNSET = 4;
module.exports = function keyedContainer(baseObj, path, keyName, valueName) {
keyName = keyName || 'name';
valueName = valueName || 'value';
var i, arr;
var i, arr, baseProp;
var changeTypes = {};

if(path && path.length) { arr = nestedProperty(baseObj, path).get();
if(path && path.length) {
baseProp = nestedProperty(baseObj, path);
arr = baseProp.get();
} else {
arr = baseObj;
}

path = path || '';
arr = arr || [];

// Construct an index:
var indexLookup = {};
for(i = 0; i < arr.length; i++) {
indexLookup[arr[i][keyName]] = i;
if(arr) {
for(i = 0; i < arr.length; i++) {
indexLookup[arr[i][keyName]] = i;
}
}

var isSimpleValueProp = SIMPLE_PROPERTY_REGEX.test(valueName);

var obj = {
// NB: this does not actually modify the baseObj
set: function(name, value) {
var changeType = value === null ? UNSET : NONE;

// create the base array if necessary
if(!arr) {
if(!baseProp || changeType === UNSET) return;

arr = [];
baseProp.set(arr);
}

var idx = indexLookup[name];
if(idx === undefined) {
if(changeType === UNSET) return;

changeType = changeType | BOTH;
idx = arr.length;
indexLookup[name] = idx;
Expand Down Expand Up @@ -86,6 +98,8 @@ module.exports = function keyedContainer(baseObj, path, keyName, valueName) {
return obj;
},
get: function(name) {
if(!arr) return;

var idx = indexLookup[name];

if(idx === undefined) {
Expand Down
101 changes: 22 additions & 79 deletions src/lib/nested_property.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]')
Expand Down Expand Up @@ -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)$/;
Copy link
Contributor

@etpinard etpinard Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

  • check that the old toolpanel does not break

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover @alexcjohnson, what if in the very unlikely scenario of someone writing in that their app relies on the old pruning behavior, what should we do?

I suspect reverting this commit would break some test(s) added in this PR (which one by the way?), so maybe we could formulate a solution for that hypothetical user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I made this commit now was the trace._length = null, which in some cases was getting pruned later on in supplyDefaults by attributes with dflt: null and breaking my redraw all the things test addition. I could have handled this in a few different ways:

  • use something other than null for _length - I considered false but to me that had the implication of "has no length" rather than what I wanted, "(1D) length does not apply." Anyway the whole idea that we're pruning in the middle of supplyDefaults which is really only supposed to be building the _full* objects I wanted to avoid.
  • make a special code path that does no pruning or unsetting - like nestedProperty.onlySet(val) - but that's adding complexity so not ideal.
  • get rid of all dflt: null (and perhaps even enforce that) throughout our attributes. I still think we should do that, but I opted not to do it here.

Anyway, to the question:

what if in the very unlikely scenario of someone writing in that their app relies on the old pruning behavior, what should we do?

If it leads to a visible change in the plot, we should fix it so the API does not contain such a dependence on the existence of empty containers ;)
If it just alters the data structures (data and layout I guess, after a restyle/relayout) I can't really predict why a user would be bothered by this, but we can help them wrap their restyle/relayout with something like if(val === null) { pruneContainersPulledFromThisCommit(...) }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great answer. Thanks!

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) {
Expand Down Expand Up @@ -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;
};
Expand Down Expand Up @@ -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'; },
Expand Down
19 changes: 10 additions & 9 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2267,7 +2267,10 @@ exports.react = function(gd, data, layout, config) {
gd.layout = layout || {};
helpers.cleanLayout(gd.layout);

Plots.supplyDefaults(gd);
// "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, {skipUpdateCalc: true});

var newFullData = gd._fullData;
var newFullLayout = gd._fullLayout;
Expand All @@ -2289,6 +2292,9 @@ exports.react = function(gd, data, layout, config) {

// clear calcdata if required
if(restyleFlags.calc || relayoutFlags.calc) gd.calcdata = undefined;
// otherwise do the calcdata updates and calcTransform array remaps that we skipped earlier
else Plots.supplyDefaultsUpdateCalc(gd.calcdata, newFullData);

if(relayoutFlags.margins) helpers.clearAxisAutomargins(gd);

// Note: what restyle/relayout use impliedEdits and clearAxisTypes for
Expand Down Expand Up @@ -2397,7 +2403,7 @@ function diffData(gd, oldFullData, newFullData, immutable) {
Axes.getFromId(gd, trace.xaxis).autorange ||
Axes.getFromId(gd, trace.yaxis).autorange
) : false;
getDiffFlags(oldFullData[i], trace, [], diffOpts);
getDiffFlags(oldFullData[i]._fullInput, trace, [], diffOpts);
}

if(flags.calc || flags.plot || flags.calcIfAutorange) {
Expand Down Expand Up @@ -2461,13 +2467,6 @@ function getDiffFlags(oldContainer, newContainer, outerparts, opts) {
return valObject.valType === 'data_array' || valObject.arrayOk;
}

// for transforms: look at _fullInput rather than the transform result, which often
// contains generated arrays.
var newFullInput = newContainer._fullInput;
var oldFullInput = oldContainer._fullInput;
if(newFullInput && newFullInput !== newContainer) newContainer = newFullInput;
if(oldFullInput && oldFullInput !== oldContainer) oldContainer = oldFullInput;

for(key in oldContainer) {
// short-circuit based on previous calls or previous keys that already maximized the pathway
if(flags.calc) return;
Expand All @@ -2494,6 +2493,8 @@ function getDiffFlags(oldContainer, newContainer, outerparts, opts) {
// in case type changed, we may not even *have* a valObject.
if(!valObject) continue;

if(valObject._compareAsJSON && JSON.stringify(oldVal) === JSON.stringify(newVal)) continue;

var valType = valObject.valType;
var i;

Expand Down
Loading