Add guards for unsafe dereferencing (ref #2687) #2757
Conversation
jacobq
commented
Jun 27, 2018
•
jacobq
commented
- In drawData
- In supplyDefaultsUpdateCalc
jacobq
force-pushed
the
fix-draw-data-exception
branch
from
b1164e7 to
51861ee
Compare
* In drawData * In supplyDefaultsUpdateCalc
jacobq
force-pushed
the
fix-draw-data-exception
branch
from
51861ee to
d8925f7
Compare
|
OK, I think this is ready for review (I'm done doing |
|
Thanks very much for the PR! I tested out https://codepen.io/watashiku/pen/pKJJNW?editors=1010 (from #2687 (comment)) off your branch and your patch didn't appear to fix the issues reported, it only hides the console errors. Moreover, your patch made a few of our tests fail. I hope I'm wrong, but I think fixing the issues of #2687, #2643, and #2644 will have to wait a multi-week effort to implement #2644 (comment) |
src/plots/plots.js
Outdated
| @@ -484,7 +484,7 @@ plots.supplyDefaults = function(gd, opts) { | |||
| plots.supplyDefaultsUpdateCalc = function(oldCalcdata, newFullData) { | |||
| for(var i = 0; i < newFullData.length; i++) { | |||
| var newTrace = newFullData[i]; | |||
| var cd0 = oldCalcdata[i][0]; | |||
| var cd0 = oldCalcdata && oldCalcdata[i] && !oldCalcdata[i][0]; | |||
There was a problem hiding this comment.
Eek, I just noticed a really bad typo here: there shouldn't be a !
|
@etpinard Please check/re-run that latest CI build. It looks to me like the failures aren't being caused by the changes in this PR (got stuck on "install dependencies" step). I'm curious to see if the test failures you saw earlier were because of my typo (had accidentally put a You are right that this PR doesn't resolve the larger issue. It's just intended to prevent uncaught exceptions. |
... and that's why I'd vote 👎 on merging this patch. |
|
If you don't want to merge it that's fine, but I don't understand how broken code is better than gracefully failing code. Of course it would be best not to have the errors at all, but if we're going to have them wouldn't it be better to handle them? |
Because I don't think adding fallbacks to these broken things doesn't make plotly.js more graceful in my mind. I could be wrong though. |
