fix: heatmap snap domain to interval

[markov00]

Summary

Heatmaps with a time scale on the X-axis now adjust the rendered time range to fully cover the edges when a custom domain is used.
We also took this opportunity to clean and abstract the code used for computing and handling date and time.
The library is now able to abstract from the underlying implementation library (moment or luxon at the moment), allowing us to experiment and work with diverse libraries removing some tech debt.

Jul-20-2021.16-14-24.mp4

Details

I've abstracted some time logic as asked by @monfera taking inspiration(copying) from what was done in the timeslip code.
I've also introduced some specific Elasticsearch types like the CalendarInterval and FixedInterval. I've made them ES specific as the concept and the snapping logic applies the same strategy as the one described in https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-datehistogram-aggregation.html

Issues

fix #1165

Checklist

• The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
• The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
• Whenever possible, please check if the closing issue is connected to a running GH project
• Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
• Proper documentation or storybook story was added for features that require explanation or tutorials
• Unit tests were updated or added to match the most common scenarios

[nickofthyme]

Looking good to me. I left a few comments below.

[monfera]

👍

[monfera]

Eelsewhere we have export type TimeMs = number; (that's for duration). So a S or Ms postfix may be useful.

It might be worth putting both this and the existing TimeMs into a common time type file (even outside /charts). The latter could perhaps be renamed DurationMs.

[monfera]

The type of the 1st and 2nd params of bisectLeft need to match, and we're typeguarding the 2nd param to number, so it's slightly tighter to say as number[]. Though it's not truthful, maybe we could brainstorm so I can learn about whether OrdinalDomain needs to be (number | string)[] and can't be number[] | string[] eg. by coercing numbers to strings if at least one element is a string. Though there's still the lack of type coherence between xValues and startValue - probably they correlate strongly (otherwise we can't do as any or as number[] here) but this seems to be lost on TS

[monfera]

If the type of bisectLeft is incorrect or too restrictive (we can't overrule it as it's beyond what the D3 TS API guarantees, but iirc these are 3rd party post hoc libs, not canonical Bostock contract) then maybe it'd be better to factor it out into our bisectLeft utility with the desired types, so we can easily change from D3 if needed. I think we could already use the also logarithmically bisecting monotonicHillClimb instead of d3.bisectLeft

[markov00]

There is one main reason for having (number | string)[] as OrdinalDomain: we like to preserve the original type, coming from the user data, to return the original values on the interaction callbacks (brushing or clicking)
We can probably force it to be number[] | string[] but I don't have strong opinion here

[monfera]

Some more info may strengthen the case for string[] | number[]:

D3 bisectLeft, and generally, logarithmic search requires total order, which we likely ensure by sorting the values beforehand(*).

Out of the properties of total order, transitivity is most important: if a <= b and b <= c then a <= c must be true (it's not necessarily true for partial orders, eg. when a number is defined non-comparable to a string).

(*) The mechanism doesn't matter, eg. [].sort also relies on a predicate plus array which together guarantee a total order, otherwise the order is ill defined: EcmaScript requires transitivity for predictable results: If a <CF b and b <CF c, then a <CF c (transitivity of <CF).

Sorting (number | string)[] with a < predicate doesn't lead to total order. Example:

a = '2'
b = 2.5
c = '10'

a < b: true
b < c: true
a < c: false

So a sensible option is to string-compare all elements if at least one element in the array is not a number, which is the default sort predicate (and number-compare for all-numeric arrays, which is not the default sort predicate). Instead of checking for this at the place of sorting, we may as well come clean and convert upfront. As you suggested Marco, the original value can still be retained. The effect of the conversion though is that a user-supplied [5, '5'] will map to ['5', '5'] so an equally good method is, documenting in the API, perhaps even enforcing via TS, that the domain values be homogeneous.

There can of course be other sorting rules, eg. all strings are moved to the end (tiebreaker rule for when a string and a number are compared), and within both the strings and numbers, there's full order

[monfera]

LGTM in general, there's an as yet unexpected mock change, and maybe we could do the bisecting differently. Glad to add a bisectLeft-like thin wrapper of the lower level monotonicHillClimb function we already have for logarithmic search in an array

[monfera]

As it's quite a long switch, requiring up to 14 comparisons, it'd be a bit nicer if these were in a map, eg.

const esCalendarIntervalsToMoment = { 
  minute: 'minutes',
  m: 'minutes',
  hour: 'hour',
  h: 'hour',
  ...
}

...

const whatever = esCalendarIntervalsToMoment[interval.unit];

[markov00]

done in f471fd0

[nickofthyme]

LGTM, still not entirely sure what the end game is having multiple time libraries.

[nickofthyme]

nit

Suggested change

	default:
	return 'hour';
	}
	case 'hour':
	case 'h':
	default:
	return 'hour';
	}

[markov00]

I've update the logic to use a map instead of a switch as proposed by robert

[nickofthyme]

I'm really not sure why this is needed as this is not configurable from outside of charts.

As it is we use moment across the charts code so are you thinking of being able to swap time libraries entirely? In that case we could have optional dependencies on moment and luxon so the user can switch between them.

If not is this to only be used as a utility to handle different time types?

[markov00]

This was done in the hope we can switch soon and easily to luxon or to Temporal https://tc39.es/proposal-temporal/docs/index.html
It really depends on Kibana right now, so extracting everything like that can help us migrating easily