feat: Add findNearest second arg to TimeScale#timeToCoordinate()

[JonDum]

Type of PR: Bugfix/Feature

PR checklist:

• Addresses an existing issue: fixes #
• Includes tests
• Documentation update

Overview of change:

There's currently a pretty major limitation with TimeScale#timeToCoordinate()

If you have data that's of a smaller interval than the timescale, timeToCoordinate() always returns null even though internally it still performs the binary search and finds the correct closest index.

I think in this scenario it'd be much better to return the closest bar than it is to return nothing at all. Or, at the very least allow the user the option of choosing by adding a second argument to timeToCoordinate().

Personally, I'd be ok with defaulting findNearest to true instead of false, since I doubt anyone would be surprised if it returns the containing bar/index instead of null, but at least with an argument the user has the option.

Additional context

My simple use case: I am writing custom pane views to show order markers. Even if the main series is set to daily bars or higher I'd still like to be able to show the orders.

[SlicedSilver]

I understand your thoughts that it would be more helpful if it found and returned the closest match, however the API is specifically designed to return null when there isn't an exact match. Its purpose isn't to find the closest but rather the exact 1-to-1 coordinate for that specific time and it is likely that existing code relies on this behaviour.

The one assumption that can currently be made is that each time value should have a unique coordinate, making this change would break this assumption. However, you could argue that having it as an optional (false) parameter means that this wouldn't be a breaking change.

The crux of the matter is that you need a way to find the closest actual time value existing on the chart. This could be achieved outside of the library because you have the data and could implement your own logic to find what would be the closest actual time value. Once you have that then you could use the timeToCoordinate method to find out the coordinate on the chart.

I would expect that if we were to add the change suggested in the PR that there would be further requests to customise how it finds the nearest point. Such as using something like: MismatchDirection

[JonDum]

From a purest standpoint, I 100% agree with you.

From a practical standpoint, I don't see the harm in allowing developers more flexibility. The needed code and logic is already in LWC — having to now have additional code for accomplishing the same thing isn't very dry.

Since this is from within a plugin that I would like to open source for the community, I do not have control over how the end user formats their data. In a situation like this I think it's reasonable to display something when they attach the plugin instead of nothing because it returns null and they begin to wonder if it's broken or they did something wrong.

[SlicedSilver]

A plugin would actually be able to access the data for the associated series. The attached lifecycle hook provides the ISeriesAPI for the series and the api does include a data() method to get the data of the series. Additionally there is a subscribeDataChanged event which you could subscribe to so you know when the data is changed in anyway.

But that is more of a side discussion. Focusing on the specific request to add the second parameter to the timeToCoordinate function. I'm not entirely against it, I just first wanted to explore if it was truly required. We are trying to be careful with what is added to the API so we can keep the library small, and not get feature blot.

In this case, the file size increase is almost zero so we can add it as a convenience.

Could you update the code in src/api/itime-scale-api.ts as well? Also adding an appropriate description to the JSDoc comment above the method. These comments are used within the documentation site.