joewalnes / smoothie

Smoothie Charts: smooooooth JavaScript charts for realtime streaming data
http://smoothiecharts.org
Other
2.24k stars 229 forks source link

NaN value of time breaks dropOldData causing performance lag #114

Closed timdrysdale closed 4 years ago

timdrysdale commented 4 years ago

Adding a point with NaN time prevents dropOldData from dropping any more data. Eventually eventually the data array grows so large the page starts lagging.

The obvious fix is to sanitise data before passing it to the library, which is what I did when I last ran into this problem. Fast forward a few years until this week when I was knocking up a quick and dirty demo and fell into the trap again and it took a couple of hours to figure out why (for the second time). So I'd thought I'd mention it here for 2yr+ future self's benefit the third time I run into this having forgotten about it yet again.

Since it is a fast lightweight library I wasn't sure if you would want to take a PR for a fix for potential user stupidity, but happy to submit a pull request if you wanted something to address it ... which might go something like this:

@@ -243,7 +244,7 @@
     // We must always keep one expired data point as we need this to draw the
     // line that comes into the chart from the left, but any points prior to that can be removed.
     var removeCount = 0;
-    while (this.data.length - removeCount >= maxDataSetLength && this.data[removeCount + 1][0] < oldestValidTime) {
+    while (this.data.length - removeCount >= maxDataSetLength && (this.data[removeCount + 1][0] < oldestValidTime || isNaN(this.data[removeCount + 1][0])) {
       removeCount++;
     }
     if (removeCount !== 0) {
drewnoakes commented 4 years ago

The library shouldn't blow up like this based on input data. It might make sense to just filter any NaN values when adding to the series. A PR would be great.

timdrysdale commented 4 years ago

Fair point to sort it on the append() - done. Tested on a demo app, and made the PR. Many thanks for a speedy response :-).