-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Transform react #2577
Changes from 8 commits
bbe3533
ed24765
fcc459d
7044a13
88b7b43
cf4c9c3
e84d4b9
b436d52
2a41f9e
965bcfb
6fae229
79295f1
5e9aa65
690eb95
a244cec
92bd5d2
dc6de2f
03956e1
f439e41
e47e6a9
aa30ad6
4c70826
7279b55
3e250df
cd08479
0414147
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)$/; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I made this commit now was the
Anyway, to the question:
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 ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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'; }, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a big fan of this pattern. At the very least we should turn this into Personally, I'd vote for removing the newly added On where the calls in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the name So I'm going to keep it as part of the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cleaner |
||
|
||
var newFullData = gd._fullData; | ||
var newFullLayout = gd._fullLayout; | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -274,7 +274,7 @@ var extraFormatKeys = [ | |
// gd._fullLayout._transformModules | ||
// is a list of all the transform modules invoked. | ||
// | ||
plots.supplyDefaults = function(gd) { | ||
plots.supplyDefaults = function(gd, skipCalcUpdate) { | ||
var oldFullLayout = gd._fullLayout || {}; | ||
|
||
if(oldFullLayout._skipDefaults) { | ||
|
@@ -458,24 +458,28 @@ plots.supplyDefaults = function(gd) { | |
} | ||
|
||
// update object references in calcdata | ||
if(oldCalcdata.length === newFullData.length) { | ||
for(i = 0; i < newFullData.length; i++) { | ||
var newTrace = newFullData[i]; | ||
var cd0 = oldCalcdata[i][0]; | ||
if(cd0 && cd0.trace) { | ||
if(cd0.trace._hasCalcTransform) { | ||
remapTransformedArrays(cd0, newTrace); | ||
} else { | ||
cd0.trace = newTrace; | ||
} | ||
} | ||
} | ||
if(!skipCalcUpdate && oldCalcdata.length === newFullData.length) { | ||
plots.supplyDefaultsUpdateCalc(oldCalcdata, newFullData); | ||
} | ||
|
||
// sort base plot modules for consistent ordering | ||
newFullLayout._basePlotModules.sort(sortBasePlotModules); | ||
}; | ||
|
||
plots.supplyDefaultsUpdateCalc = function(oldCalcdata, newFullData) { | ||
for(var i = 0; i < newFullData.length; i++) { | ||
var newTrace = newFullData[i]; | ||
var cd0 = oldCalcdata[i][0]; | ||
if(cd0 && cd0.trace) { | ||
if(cd0.trace._hasCalcTransform) { | ||
remapTransformedArrays(cd0, newTrace); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. merged and 🐎 |
||
} else { | ||
cd0.trace = newTrace; | ||
} | ||
} | ||
} | ||
}; | ||
|
||
/** | ||
* Make a container for collecting subplots we need to display. | ||
* | ||
|
@@ -1174,6 +1178,11 @@ plots.supplyTraceDefaults = function(traceIn, colorIndex, layout, traceInIndex) | |
}; | ||
|
||
plots.supplyTransformDefaults = function(traceIn, traceOut, layout) { | ||
// For now we only allow transforms on 1D traces, ie those that specify a _length. | ||
// If we were to implement 2D transforms, we'd need to have each transform | ||
// describe its own applicability and disable itself when it doesn't apply. | ||
if(!traceOut._length) return; | ||
|
||
var globalTransforms = layout._globalTransforms || []; | ||
var transformModules = layout._transformModules || []; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,19 +38,28 @@ function supplyDefaults(traceIn, traceOut, defaultColor, layout) { | |
function handleSampleDefaults(traceIn, traceOut, coerce, layout) { | ||
var y = coerce('y'); | ||
var x = coerce('x'); | ||
var hasX = x && x.length; | ||
|
||
var defaultOrientation; | ||
var defaultOrientation, len; | ||
|
||
if(y && y.length) { | ||
defaultOrientation = 'v'; | ||
if(!x) coerce('x0'); | ||
if(hasX) { | ||
len = Math.min(x.length, y.length); | ||
} | ||
else { | ||
coerce('x0'); | ||
len = y.length; | ||
} | ||
} else if(x && x.length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be |
||
defaultOrientation = 'h'; | ||
coerce('y0'); | ||
len = x.length; | ||
} else { | ||
traceOut.visible = false; | ||
return; | ||
} | ||
traceOut._length = len; | ||
|
||
var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleTraceDefaults'); | ||
handleCalendarDefaults(traceIn, traceOut, ['x', 'y'], layout); | ||
|
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2a41f9e ->
isArray1D