test(xy): scale type improvements #1381

monfera · 2021-09-16T09:19:24Z

Summary

As a first step for comprehensive type safety, this PR increases TS type safety to the central Cartesian functions/methods/classes and lots of utility and render function dependencies through turning the Scale generic on its domain

Details

There were a couple of strategically placed anys in the Scale interface, and elsewhere. Though low in number, their propagating effect meant that most of the xy code doesn't really enjoy type safety, there can always be a scale induced any leak, so only very specific functions could be considered type safe.

The current PR is a start of work, focusing on Scale.domain, Scale.ticks, and its positive effects on Scale methods and lots of other functions. While there's still a lot of dirt swept under the carpet (just grep as Scale<) it's way more explicit, and these assertions can be solved one by one.

So, statically a lot more places where we assert or say any, but it's more explicit, closer to the as yet unresolved bits.

There's also a place where the D3 scale is called with a type that's not enabled ([undefined] domain); it's now marked explicitly.

Issues

Under the umbrella of #1303

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
All related issues have been linked (i.e. closes #123, fixes #123)
Unit tests have been added or updated to match the most common scenarios

monfera · 2021-09-16T16:44:25Z

test this

packages/charts/src/chart_types/xy_chart/annotations/line/dimensions.ts

packages/charts/src/chart_types/xy_chart/annotations/rect/dimensions.ts

packages/charts/src/chart_types/xy_chart/state/selectors/compute_annotations.ts

packages/charts/src/scales/index.ts

packages/charts/src/scales/types.ts

markov00

I haven't gone too far, but in general this are the current assumptions for cartesian:
yScale is always Scale<number>
xScale is always Scale<number|string>, can be restricted if required but most of the time I don't need to know if the value I'm sending to the scale is a number or a string, I just need to know that the output of the scale is a number

markov00 · 2021-09-16T17:34:14Z

packages/charts/src/chart_types/xy_chart/annotations/line/dimensions.ts

+        const padding = (xScale.step - (xScale as ScaleBand<string | number>).originalBandwidth) / 2;
        annotationValueXPosition -= padding;
      } else {
-        annotationValueXPosition += xScale.originalBandwidth / 2;
+        annotationValueXPosition += (xScale as ScaleBand<string | number>).originalBandwidth / 2;


I think you can remove this assertion because isBandScale already define xScale as ScaleBand

Thanks, will check! I haven't added assertions where there was no complaint, but it's possible that a later commit made an earlier assertion unnecessary, indeed a good idea to revisit all

Indeed, I only later turned the type guard function generic, now it no longer complains there 👯‍♀️

markov00 · 2021-09-16T17:37:34Z

packages/charts/src/chart_types/xy_chart/annotations/utils.ts

+      const dimensions = computeRectAnnotationDimensions(
+        annotationSpec,
+        yScales,
+        xScale as Scale<number>,


why you are restricting this to number only if the passed one could be both?

You can change computeRectAnnotationDimensions to accept Scale<unknown> because internally you are calling isBandScale that assert the type for you

Re the question, it's because it's in a conditional branch, and I didn't want to add potentially unduly restrictive types in the other, unrelated branch in the first round of improvements

I can't change computeRectAnnotationDimensions to accept Scale<unknown> because calling limitValueToDomainRange will have a red squiggly mark (limitValueToDomainRange performs a subtraction of domain values, so it must be numeric) and for the same reason it can't be Scale<number | string> either

Basically the computeRectAnnotationDimensions only seems to work with numeric X, though I haven't tested the runtime, it was enough for me to see the subtraction. Though the subtraction only happens if the domain length is non-zero, I assumed it's always the case

By extension, the entire computeRectAnnotationDimensions only works with a numeric X scale, Marco or Nick lmk if it's not the case

markov00 · 2021-09-16T17:40:26Z

packages/charts/src/chart_types/xy_chart/annotations/rect/dimensions.ts

@@ -64,7 +64,7 @@ export function computeRectAnnotationDimensions(
    let xAndWidth: { x: number; width: number } | null = null;

    if (isBandScale(xScale)) {
-      xAndWidth = scaleXonBandScale(xScale, x0, x1);
+      xAndWidth = scaleXonBandScale(xScale as ScaleBand<number>, x0, x1);


this one also seems unnecessary, see my previous comment in file /xy_chart/annotations/utils.ts

Yes, thanks for the catch, I need to revisit these (the assertion was done before the function param types were tightened)

markov00 · 2021-09-16T17:43:46Z

packages/charts/src/chart_types/xy_chart/rendering/area.ts

@@ -31,8 +31,8 @@ import {
 export function renderArea(
  shift: number,
  dataSeries: DataSeries,
-  xScale: Scale,
-  yScale: Scale,
+  xScale: Scale<number>,


this should be string|number as every other xScale
The scale is used in the pathGenerator.x function with the data coming from dataSeries.data where the x can be both string or number DataSeriesDatum.x

xScale? are we ever linking ordinals with a line? I tightened it to number because:

it unconditionally calls renderPoints which expects numeric

I wasn't aware of using lines for ordinals

tests let it pass

Just made a quick attempt to relax the xScale param of renderPoints to also allow strings and it didn't complain, so I can relax it here too, though no idea why I didn't get a complaint for my exclusion of the strings, will look into it

markov00 · 2021-09-16T17:44:24Z

packages/charts/src/chart_types/xy_chart/rendering/bubble.ts

@@ -21,8 +21,8 @@ import { MarkSizeOptions } from './utils';
 export function renderBubble(
  shift: number,
  dataSeries: DataSeries,
-  xScale: Scale,
-  yScale: Scale,
+  xScale: Scale<number>,


number|string

markov00 · 2021-09-16T17:44:32Z

packages/charts/src/chart_types/xy_chart/rendering/line.ts

@@ -30,8 +30,8 @@ import {
 export function renderLine(
  shift: number,
  dataSeries: DataSeries,
-  xScale: Scale,
-  yScale: Scale,
+  xScale: Scale<number>,


number|string

markov00 · 2021-09-16T17:44:42Z

packages/charts/src/chart_types/xy_chart/rendering/points.ts

@@ -29,8 +29,8 @@ import {
 export function renderPoints(
  shift: number,
  dataSeries: DataSeries,
-  xScale: Scale,
-  yScale: Scale,
+  xScale: Scale<number>,


number|string

Thanks Marco, will revisit these!

Co-authored-by: Nick Partridge <[email protected]>

monfera · 2021-09-16T20:10:27Z

packages/charts/src/chart_types/xy_chart/state/selectors/compute_small_multiple_scales.ts

-  const defaultDomain = domain.length === 0 ? [undefined] : domain;
-  return new ScaleBand(defaultDomain, [0, maxRange], undefined, singlePanelSmallMultiple ? 0 : padding);
+  return new ScaleBand(
+    domain.length > 0 ? domain : [(undefined as unknown) as number],


Yes we've already been cheating to comply with the D3 scale typedef file and the D3 doc, but now we're clear about it. I'll try to solve it in a next PR, it's only partially addressed in this one

packages/charts/src/scales/scale_band.ts

monfera · 2021-09-16T20:18:12Z

packages/charts/src/utils/events.ts

-  }
-  return false;
+  const nextLastDrag = nextProps.lastDrag;
+  return nextLastDrag !== null && (prevLastDrag === null || prevLastDrag.end.time !== nextLastDrag.end.time);


Replacing a hairy control structure and making the logical conditions less redundant

nickofthyme · 2021-10-05T10:16:56Z

🎉 This PR is included in version 37.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

monfera added the wip work in progress label Sep 16, 2021

monfera marked this pull request as draft September 16, 2021 09:19

chore: tick values are numbers

543c33c

monfera force-pushed the time-axis-05 branch from 6306ebf to a1b8a59 Compare September 16, 2021 11:40

monfera added 11 commits September 16, 2021 14:25

chore: showing any via the generic type

915c86f

chore: converting more any to unknown

9ed546a

chore: converting more any to unknown 02

12ef5a8

chore: converting more any to unknown 03

c7f89b2

chore: converting more any to unknown 04

26d9935

chore: converting more any to unknown 05

8675664

chore: converting more any to real type 06

2ae39b3

chore: converting more any to real type 07

57c5c8f

chore: converting more any to real type 08

3607f0a

chore: converting more any to real type 09

ae17f5c

chore: converting more any to real type 10

bb93b5a

monfera force-pushed the time-axis-05 branch from a1b8a59 to bb93b5a Compare September 16, 2021 12:25

monfera added 5 commits September 16, 2021 14:33

chore: converting more any to real type 11

432238d

chore: converting more any to real type 12

9be0b6a

chore: generic ScaleBand 01

770f3a6

chore: generic ScaleBand 02

44af026

chore: generic ScaleBand 03

6eea0f7

monfera marked this pull request as ready for review September 16, 2021 16:37

monfera added 2 commits September 16, 2021 18:39

chore: converting more any to real type 13

0ec33ef

chore: converting more any to real type 14

7272653

monfera added 3 commits September 16, 2021 18:54

chore: converting more any to real type 15

5bc5c0a

chore: converting more any to real type 16

3e30380

refactor: drive by removal of verbose code

09bed4a

monfera changed the title ~~chore: test~~ test(xy): scale type improvements Sep 16, 2021

monfera removed the wip work in progress label Sep 16, 2021

monfera added :refactor Code refactor :test Missing or to be fixed test :xy Bar/Line/Area chart related code quality labels Sep 16, 2021

chore: converting more any to real type 17

c20cef9

monfera requested review from markov00, nickofthyme and rshen91 September 16, 2021 17:30

monfera added the :data Data/series/scales related issue label Sep 16, 2021

monfera mentioned this pull request Sep 16, 2021

Code improvements meta issue #1303

Open

19 tasks

nickofthyme reviewed Sep 16, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/annotations/line/dimensions.ts Outdated Show resolved Hide resolved

nickofthyme reviewed Sep 16, 2021

View reviewed changes

markov00 reviewed Sep 16, 2021

View reviewed changes

monfera and others added 4 commits September 16, 2021 20:36

test: type improvement round ht MarcoV NickP

0ac3fd2

test: generic typing for isBandScale

8cd6cda

Co-authored-by: Nick Partridge <[email protected]>

test: doing away with the unknown

afaceb6

test: more assertion removals due to review update ripple-down

b59e2e0

monfera requested review from markov00 and nickofthyme September 16, 2021 19:40

monfera added 2 commits September 16, 2021 22:00

test: more any removals

b36aefb

test: all yScale occurrences are numeric

640d644

monfera commented Sep 16, 2021

View reviewed changes

test: minor other unifications

36d1596

monfera commented Sep 16, 2021

View reviewed changes

packages/charts/src/scales/scale_band.ts Show resolved Hide resolved

monfera commented Sep 16, 2021

View reviewed changes

nickofthyme approved these changes Sep 16, 2021

View reviewed changes

monfera merged commit cd201de into elastic:master Sep 16, 2021

nickofthyme added the released Issue released publicly label Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(xy): scale type improvements #1381

test(xy): scale type improvements #1381

monfera commented Sep 16, 2021 •

edited

Loading

monfera commented Sep 16, 2021

markov00 left a comment

markov00 Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

markov00 Sep 16, 2021

markov00 Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

markov00 Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

markov00 Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

markov00 Sep 16, 2021

monfera Sep 16, 2021

markov00 Sep 16, 2021

monfera Sep 16, 2021

markov00 Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021

monfera Sep 16, 2021 •

edited

Loading

monfera Sep 16, 2021

nickofthyme commented Oct 5, 2021

test(xy): scale type improvements #1381

test(xy): scale type improvements #1381

Conversation

monfera commented Sep 16, 2021 • edited Loading

Summary

Details

Issues

Checklist

monfera commented Sep 16, 2021

markov00 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickofthyme commented Oct 5, 2021

monfera commented Sep 16, 2021 •

edited

Loading

monfera Sep 16, 2021 •

edited

Loading