fix(deps): update dependency d3-scale to v3.3.0 #1669

watson · 2022-05-10T19:08:27Z

I'm performing this upgrade to get a newer version of d3-color which is a direct dependency of d3-scale.

I think this is the only commit to d3-scale v2.0.0 with potentially breaking changes: d3/d3-scale@7819336

I think that we should be ok with that in this package, but hopefully CI will let us know.

To Reviewers

This PR also fixes the issue listed here: #1095
The new d3 scale version project a datapoint to the center of its range if the domain min === domain max see d3/d3-scale#117

nickofthyme

LGTM provided the visual regression checks pass. 👍🏼

watson · 2022-05-11T06:11:54Z

I'm not really sure what failed with the @elastic/charts CI check (I think it's just an intermittent failure in CI). But I'm not sure how to make it re-run?

watson · 2022-05-11T09:11:29Z

Turns out after I updated the types, that CI fails due to apparent minor changes in the return values of the d3-scale API. I think this is just that the types were always a little bit off and have not just been corrected. But I'm not 100% sure. I'll investigate

watson · 2022-05-11T09:17:15Z

After looking over the TypeScript failures introduced by this PR, I think it's outside my scope of expertise to fix. Would anyone from the @elastic/datavis team be able to help me fix the CI issues?

markov00 · 2022-05-11T09:30:46Z

hey @watson I will take a look at this.
One question: do we need to stick to that version 2.2.2 or can we also upgrade to something more up to date?
We are stuck with such old versions because newer versions are not ES5 compatible and at that time, Kibana was transpiling to es5 but was not transpiling node modules.
If that was changed, I'm happy to upgrade all the d3 deps

watson · 2022-05-11T10:39:56Z

@markov00 Thanks ❤️

Ideally I'd love to upgrade to the latest version (v4.0.2), but I wasn't sure if that was going to be too big of an upgrade in one PR, so I kept it to v2.x in this one. However, if you feel it's not a problem, it would be great to upgrade all the way to v4.x.

watson · 2022-05-11T13:16:15Z

Regarding Kibana and ES modules: Kibana still cannot process ES module style packages. However, this is only an issue if they are direct dependencies. We already have sevel sub-dependencies that are of the ES module variant. So upgrading d3-scale here to an ES module (which v4.x is), shouldn't be a problem for Kibana as it will going to be a sub-dependency and not a direct dependency.

BREAKING CHANGE: the new d3-scale version used ES2015 and our target version needs to align to that.

markov00 · 2022-05-17T10:15:45Z

Hi @watson sorry for delaying this PR so much, but I'm getting some trouble fixing the new behavior and types of d3-scale. Scales are probably the most important and delicate part of our library so I need to fix those changes carefully.

Upgrading to something 4.x also creates issues at the Jest level, because v4.x used ESM modules and we don't transpile node_modules in our build chain. I will stick to 3.x version for now

watson · 2022-05-17T13:34:25Z

Thanks for taking this one on @markov00 and no worries about the delays. We'll get there eventually.

Upgrading to something 4.x also creates issues at the Jest level, because v4.x used ESM modules and we don't transpile node_modules in our build chain. I will stick to 3.x version for now

Sounds good 👍 The most important part was upgrading to v2.2.2 or newer.

watson · 2022-05-31T09:54:49Z

Thank you @markov00 for doing all the hard work in making this PR all green!! ❤️❤️❤️

monfera

LGTM! Added minor comments, maybe the union type avoidance with this.project is actionable in this PR or, if planned, a follow-up one. If the price is not too high

monfera · 2022-05-31T13:18:31Z

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

@@ -47,7 +47,7 @@ function getPointerEvent(
    return { chartId, type: PointerEventType.Out };
  }
  const xValue = xScale.invertWithStep(x, geometriesIndexKeys);
-  if (!xValue) {
+  if (!Number.isFinite(xValue.value)) {


I didn't write the original if (!xValue) { and most likely an intended change: it used to evaluate to true for zeros, but not anymore.

Also, current master has four occurrences of if (!xValue) { and this PR replaces three—should the fourth one change too?

I will take a look at the fourth one.
The xValue is actually an object and the invertWithStep always returns that object, so the check was actually wrong and wasn't checking the possible NaN value returned

done here f171590

I wrongly considered that check, I will push a commit to fix it, sorry.
The invertWithStep can be also a string in case of categorical scale...

monfera · 2022-05-31T13:23:39Z

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

  const allTicks: AxisTick[] =
    makeRaster && isSingleValueScale && typeof firstTickValue === 'number'
      ? [
          {
            value: firstTickValue,
            domainClampedValue: firstTickValue,
            label: labelFormatter(firstTickValue),
-            position: (scale.scale(firstTickValue) || 0) + offset,
-            domainClampedPosition: (scale.scale(firstTickValue) || 0) + offset,
+            position: (isFiniteNumber(firstTickValueScaled) ? firstTickValueScaled : 0) + offset,


I agree that isFiniteNumber is a bit more explicit than checking for NaN. But it's more verbose (function call plus ternary). Are there signs that the code can yield non-finite numbers other than NaN, ie. the Infinities? (and non-numbers of course for potentially non-numeric types).

Code size wise, if we know it's either a finite number (incl. zero) or a NaN or other falsey thing like null ur undefined, then the value || 0 is a compact alternative to a ternary. No big deal either way, though I personally prefer smaller/simpler code

there could be potential Infinity values when, for example, the scale is a log scale and the value passed is 0.
What should we do instead? guard for such numbers inputs at the scale level? apply such finite check before returning from the scale method?

This could be simplified in cases where a default number is desired with a helper function like...

const getFinite = (v: number, fallback: number) => isFiniteNumber(v) ? v : fallback;

monfera · 2022-05-31T13:28:30Z

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

        const panelAnchor: Point = {
-          x: horizontal.scale(horizontalValue) || 0,
-          y: vertical.scale(verticalValue) || 0,
+          x: isFiniteNumber(x) ? x : 0,


A reflection on my own past refactorings too, which solved some things but didn't address this long-lived aspect: there are so many occurrences of this safety valve, it's not good practice to do these checks at the "leafs". Not for this PR, but let's think about an approach where we take care of NaNs close to where they arise, in a centralized way, so we know that it's finite everywhere downstream. Unfortunately TS is quite weak in reasoning and can't express or propagate knowledge that something will be finite, so it may not be feasible or worth it

Agree, but I don't think we have a lot of chances to get this works in TS.

monfera · 2022-05-31T13:31:00Z

packages/charts/src/scales/scale_continuous.ts

@@ -70,7 +79,7 @@ export class ScaleContinuous implements Scale<number> {
  readonly timeZone: string;
  readonly barsPadding: number;
  readonly isSingleValueHistogram: boolean;
-  private readonly project: (d: number) => number;
+  private readonly project: (d: number) => number | undefined;


What's the reason for admitting undefined? I'm not thrilled with increasing our use of ternaries if it's not hard to avoid

Also, inverseProject still doesn't admit undefined, so it became a bit of a misnomer, even if it causes no problem in code execution

done in 096e346

packages/charts/src/chart_types/xy_chart/rendering/__snapshots__/rendering.bands.test.ts.snap

packages/charts/src/chart_types/xy_chart/state/__snapshots__/chart_state.debug.test.ts.snap

nickofthyme · 2022-06-01T15:16:11Z

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

@@ -39,29 +40,30 @@ export const getElementAtCursorPositionSelector = createCustomCachedSelector(

 function getElementAtCursorPosition(
  orientedProjectedPointerPosition: PointerPosition,
-  scales: ComputedScales,
+  { xScale }: ComputedScales,


❤️ you were in a destructuring mode here!

nickofthyme · 2022-06-01T15:20:53Z

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

  const allTicks: AxisTick[] =
    makeRaster && isSingleValueScale && typeof firstTickValue === 'number'
      ? [
          {
            value: firstTickValue,
            domainClampedValue: firstTickValue,
            label: labelFormatter(firstTickValue),
-            position: (scale.scale(firstTickValue) || 0) + offset,
-            domainClampedPosition: (scale.scale(firstTickValue) || 0) + offset,
+            position: (isFiniteNumber(firstTickValueScaled) ? firstTickValueScaled : 0) + offset,


This could be simplified in cases where a default number is desired with a helper function like...

const getFinite = (v: number, fallback: number) => isFiniteNumber(v) ? v : fallback;

nickofthyme · 2022-06-01T15:36:58Z

@markov00 does this fully address #1095?

markov00 · 2022-06-01T16:32:13Z

@nickofthyme yes it solves it in both the X axis and in the Y one

markov00 · 2022-06-15T12:28:29Z

@watson sorry for the long delay in this, we have changed again the route and we merged the update of the dependency in this PR #1696

watson changed the title ~~fix(deps): updaate dependency d3-scale to v2.2.2~~ fix(deps): update dependency d3-scale to v2.2.2 May 10, 2022

watson marked this pull request as ready for review May 10, 2022 19:16

nickofthyme approved these changes May 10, 2022

View reviewed changes

fix(deps): update dependency d3-scale to v2.2.2

d0c3c24

watson force-pushed the bump-d3-scale branch from cc72fc7 to d0c3c24 Compare May 11, 2022 08:33

Update types

7a5ee49

markov00 force-pushed the bump-d3-scale branch 2 times, most recently from 12ad738 to de24f8f Compare May 12, 2022 15:44

markov00 marked this pull request as draft May 12, 2022 15:45

deps(d3): fix updated type issues

29804a6

BREAKING CHANGE: the new d3-scale version used ES2015 and our target version needs to align to that.

markov00 force-pushed the bump-d3-scale branch from de24f8f to 29804a6 Compare May 12, 2022 16:26

markov00 added 3 commits May 16, 2022 18:20

Fix scale types

569f36c

deps: downgraded d3-scale to 3.3.3

01083b6

Fix NaN usage across scales

0dcc64e

markov00 added 6 commits May 27, 2022 17:20

test: removing check for zero values fixed in elastic#1674

1e67493

test: update precision on some calculations

a9d6149

Merge branch 'master' into bump-d3-scale

45087a0

fix: remove wrong isFinite check on inverted scale

2a05922

test: add test with a single data pint

df0f6a0

test: update VRTs

4f68952

markov00 marked this pull request as ready for review May 30, 2022 13:50

markov00 requested a review from nickofthyme May 30, 2022 13:50

markov00 requested a review from monfera May 30, 2022 13:50

markov00 changed the title ~~fix(deps): update dependency d3-scale to v2.2.2~~ fix(deps): update dependency d3-scale to v3.3.0 May 31, 2022

monfera approved these changes May 31, 2022

View reviewed changes

markov00 added 2 commits June 1, 2022 10:51

refactor: prevent undefined return in scale project

096e346

fix: remove last wrong check on xValue

cf6453b

markov00 force-pushed the bump-d3-scale branch from f171590 to cf6453b Compare June 1, 2022 09:27

nickofthyme approved these changes Jun 1, 2022

View reviewed changes

nickofthyme linked an issue Jun 1, 2022 that may be closed by this pull request

Better handling of linear scale with a single data point and no domain #1095

Closed

markov00 mentioned this pull request Jun 15, 2022

refactor(scales): use specific scales instead of generics #1696

Merged

4 tasks

markov00 closed this Jun 15, 2022

watson deleted the bump-d3-scale branch July 11, 2022 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(deps): update dependency d3-scale to v3.3.0 #1669

fix(deps): update dependency d3-scale to v3.3.0 #1669

watson commented May 10, 2022 •

edited by markov00

Loading

nickofthyme left a comment

watson commented May 11, 2022

watson commented May 11, 2022

watson commented May 11, 2022

markov00 commented May 11, 2022 •

edited

Loading

watson commented May 11, 2022

watson commented May 11, 2022 •

edited

Loading

markov00 commented May 17, 2022 •

edited

Loading

watson commented May 17, 2022

watson commented May 31, 2022 •

edited

Loading

monfera left a comment

monfera May 31, 2022

markov00 May 31, 2022 •

edited

Loading

markov00 Jun 1, 2022

markov00 Jun 1, 2022

monfera May 31, 2022

markov00 May 31, 2022

nickofthyme Jun 1, 2022

monfera May 31, 2022

markov00 Jun 1, 2022

monfera May 31, 2022

monfera May 31, 2022

markov00 Jun 1, 2022

nickofthyme Jun 1, 2022

nickofthyme Jun 1, 2022

nickofthyme commented Jun 1, 2022

markov00 commented Jun 1, 2022

markov00 commented Jun 15, 2022

fix(deps): update dependency d3-scale to v3.3.0 #1669

fix(deps): update dependency d3-scale to v3.3.0 #1669

Conversation

watson commented May 10, 2022 • edited by markov00 Loading

To Reviewers

nickofthyme left a comment

Choose a reason for hiding this comment

watson commented May 11, 2022

watson commented May 11, 2022

watson commented May 11, 2022

markov00 commented May 11, 2022 • edited Loading

watson commented May 11, 2022

watson commented May 11, 2022 • edited Loading

markov00 commented May 17, 2022 • edited Loading

watson commented May 17, 2022

watson commented May 31, 2022 • edited Loading

monfera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markov00 May 31, 2022 • edited Loading

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

nickofthyme commented Jun 1, 2022

markov00 commented Jun 1, 2022

markov00 commented Jun 15, 2022

watson commented May 10, 2022 •

edited by markov00

Loading

markov00 commented May 11, 2022 •

edited

Loading

watson commented May 11, 2022 •

edited

Loading

markov00 commented May 17, 2022 •

edited

Loading

watson commented May 31, 2022 •

edited

Loading

markov00 May 31, 2022 •

edited

Loading