react-timeseries-charts icon indicating copy to clipboard operation
react-timeseries-charts copied to clipboard

EventHandler produces reversed TimeRanges

Open brandly opened this issue 5 years ago • 2 comments

🐛Bug report

Before calling onZoom, EventHandler creates the necessary TimeRange that will be passed to onZoom: https://github.com/esnet/react-timeseries-charts/blob/aa9c9b368100d78337b562d9e2833f2d90d9de3d/src/components/EventHandler.js#L206-L209

The desired behavior is to produce a TimeRange where timeRange.begin() < timeRange.end(). However, array sort in JS is... interesting. To quote MDN:

all non-undefined array elements are sorted by converting them to strings and comparing strings in UTF-16 code units order.

For dates before 1970, newBegin and newEnd will be negative numbers. In many cases, these numbers will be consistently reversed. For example, in the node repl:

$ node
Welcome to Node.js v14.4.0.
Type ".help" for more information.
> [-390040203141, -204123317277].sort()
[ -204123317277, -390040203141 ]

To Reproduce Steps to reproduce the behavior:

  1. View a ChartContainer with enableDragZoom set to true
  2. Drag to select a time range before 1970
  3. View the time range passed to your onTimeRangeChanged handler

Expected behavior The values should be properly sorted. onZoom is called in three places. Before the other two calls, no sorting takes place.

The other two places might be relying on the surrounding context for sort order, but I think it'd be best to explicitly sort in all three spots.

brandly avatar Sep 17 '20 19:09 brandly

i can send a PR. some options:

  1. do this inline
const newTimeRange = new TimeRange(newBegin < newEnd ? [newBegin, newEnd] : [newEnd, newBegin]); 
  1. pull that into a function
const ascendingDates = (a, b) =>
  a < b ? [a, b] : [b, a]

// ...

const newTimeRange = new TimeRange(ascendingDates(newBegin, newEnd)); 
  1. sort any array of numbers in place
const sortNumbers = (numbers) =>
  numbers.sort((a, b) => {
    if (a < b) return -1
    if (b > a) return 1
    return 0
  })

// ...

const newTimeRange = new TimeRange(sortNumbers([newBegin, newEnd])); 

@pjm17971 what do you think?

brandly avatar Sep 17 '20 19:09 brandly

I guess this came from a PR ages ago.

It's kind of too bad TimeRange doesn't just suck it up and order them if they are out of order. But to fix it here I guess the first approach is fine.

pjm17971 avatar Sep 21 '20 15:09 pjm17971