esnet / react-timeseries-charts

Declarative and modular timeseries charting components for React
http://software.es.net/react-timeseries-charts
Other
859 stars 283 forks source link

EventHandler produces reversed TimeRanges #454

Open brandly opened 3 years ago

brandly commented 3 years ago

🐛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 commented 3 years ago

i can send a PR. some options:

  1. do this inline

    const newTimeRange = new TimeRange(newBegin < newEnd ? [newBegin, newEnd] : [newEnd, newBegin]); 
  2. pull that into a function

    
    const ascendingDates = (a, b) =>
    a < b ? [a, b] : [b, a]

// ...

const newTimeRange = new TimeRange(ascendingDates(newBegin, newEnd));


3. sort any array of numbers in place
```js
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?

pjm17971 commented 3 years ago

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.