refactor: use redux in favour of mobx

[markov00]

Summary

Todos

• reenable legend values
• reenable external cursor events/share cursor position
• check deselectedDataSeries: DataSeriesColorsValues[];
• isCursorOnChart
• fix brush end

Breaking Changes

• SpecId,AxisId, AnnotationId types are down-casted to a string type. The getSpecId, getAxisIdm getAnnotationId methods still exist and but return just the same passed string until deprecated in a future version.
• The spec ids, previously id, axisId,annotationId etc are now aligned to use the same prop name: id
• The chart rendering status data-ech-render-complete and data-ech-render-count is no more at the root level of the echChart div, but on its child element: echChartStatus

This PR refactor the current MobX implementation of elastic-chart in favour of redux and a better API to include new chart types.

GlobalChartState

The new redux state is configured as the following:

export interface GlobalChartState {
  // an unique ID for each chart used by re-reselect to memoize selector per chart
  chartId: string;
  // true when all all the specs are parsed ad stored into the specs object
  specsInitialized: boolean;
  // true if the chart is rendered on dom
  chartRendered: boolean;
  // incremental count of the chart rendering
  chartRenderedCount: number;
  // the map of parsed specs
  specs: SpecList;
  // the chart type depending on the used specs
  chartType: ChartType | null;
  // a chart-type-dependant class that is used to render and share chart-type dependant functions
  internalChartState: InternalChartState | null;
  // the dimensions of the parent container, including the legend
  parentDimensions: Dimensions;
  // the state of the interactions
  interactions: InteractionsState;
  // external event state
  externalEvents: ExternalEventsState;
}

That global state maintain mainly the specs map (parsed from the <SpecParser /> component, similarly to the previous MobX implementation. Each spec is now stored into a plain JS object where each key is the id of the Spec.
A Spec has now few more properties

export interface Spec {
  /** unique Spec identifier */
  id: string;
  /** Chart type define the type of chart that use this spec */
  chartType: ChartType;
  /** The type of spec, can be series, axis, annotation, settings etc*/
  specType: string;
}

New Chart types

Each ChartType needs one or more Spec and needs to register its own InternalChartState class into the initInternalChartState function of src/state/chart_state.ts.
The InternalChartState class follow this interface:

export interface InternalChartState {
  // the chart type
  chartType: ChartType;
  // returns a JSX element with the chart rendered (lenged excluded)
  chartRenderer(containerRef: BackwardRef, forwardStageRef: RefObject<Stage>): JSX.Element | null;
  // true if the brush is available for this chart type
  isBrushAvailable(globalState: GlobalChartState): boolean;
  // true if the chart is empty (no data displayed)
  isChartEmpty(globalState: GlobalChartState): boolean;
  // return the list of legend items
  getLegendItems(globalState: GlobalChartState): Map<string, LegendItem>;
  // return the list of values for each legend item
  getLegendItemsValues(globalState: GlobalChartState): Map<string, TooltipLegendValue>;
  // return the CSS pointer cursor depending on the internal chart state
  getPointerCursor(globalState: GlobalChartState): string;
}

close #75

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
• This was checked for cross-browser compatibility, including a check against IE11
• 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
• Each commit follows the convention

[codecov-io]

Codecov Report

Merging #281 into master will increase coverage by 6.25%.
The diff coverage is 79.43%.

@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   77.85%   84.11%   +6.25%     
==========================================
  Files          81      168      +87     
  Lines        4209     4784     +575     
  Branches      894      917      +23     
==========================================
+ Hits         3277     4024     +747     
+ Misses        920      744     -176     
- Partials       12       16       +4

Impacted Files	Coverage Δ
src/specs/settings.tsx	100% <ø> (+5.43%)	⬆️
src/state/selectors/is_chart_empty.ts	75% <ø> (ø)
src/utils/themes/light_theme.ts	100% <ø> (ø)	⬆️
src/state/utils.ts	85.71% <ø> (ø)
src/state/chart_state.ts	79.59% <ø> (ø)
src/utils/scales/scale_band.ts	94.73% <ø> (-0.14%)	⬇️
src/state/spec_factory.ts	92.3% <ø> (ø)
src/state/reducers/interactions.ts	56.52% <ø> (ø)
src/state/actions/chart.ts	100% <ø> (ø)
...te/selectors/get_internal_is_brushing_available.ts	75% <ø> (ø)
... and 243 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8afa62a...874ebd7. Read the comment docs.

[monfera]

First of all, great changes overall toward data flow simplification!

There are a couple of places where some more uncoupling could be done, here's the main one: likely because of what chart types have been covered so far, Chart (src/components/chart.tsx) now depends on and builds an xy_chart (ie. stuff under src/chart_types/xy_chart) even when instantiating something like

<Chart>...
  <Pie...>
</Chart>

where Pie is in a separate directory under /chart_types/ (even appears to work as long as structural typing is observed but won't use the code under /chart_types/circline)

As Chart is a quite general name, and likely meant to be a top-level container for all things in elastic-charts from the API viewpoint, this coupling would ideally give way to a plurality of projection types incl. non-Cartesian ones. An alternative is to rename it eg. CartesianChart, and there may be other ways already thought out, or we all could brainstorm

[monfera]

The other uncoupling question/suggestion is about whether certain computations could be separated from views that they are often associated with. For example, get_cursor_band.ts or get_tooltip_values_highlighted_geoms.ts seems to depend on axes, which are basically, things to render only. So maybe it could be split to two parts: the computational part would have the responsibility of returning which value, band, point etc. the cursor is over; and things like the Axis, Crosshair and Tooltip would merely be responsible for rendering. Ie. no sideways communication. I see the attraction, and how a coupling can evolve historically, eg. we need a crosshair and need to compute where the cursor is in data space, and then later we add tooltips, which can reuse some values already computed for the crosshair. It'd be neat to separate because eg. some charts may need tooltips that don't need crosshair.

Another example is get_tooltip_values_highlighted_geoms.ts and tooltip.ts. Here it's coupled, and data flow is happening sideways, because the tooltip takes formatting spec defaults from the axes (maybe other things happen too or there's a deeper explanation). Perhaps the default formatters could belong to the projection (each scale, eg. xScale and yScale) rather than the axes, or at least it'd be neat to reconsider data transfers between sibling components like tooltip vs axis.

I'm not necessarily suggesting that instead of an <Axis...> tag, that currently specifies a tickFormatter, we mandate library users to use some newfangled, and for many users, hard to grasp <Projection...> tag under <Chart>, even though it'd make our technical work easier. But maybe it's possible to make it so that the <Tooltip> component need not depend on <Axis> because from the tooltip viewpoint, the tickFormat comes from "upward". For a hypothetical example, if later on, someone decides that the default tickFormat needs to come from somewhere else, eg. <Projection> or <Formatters> or <Config> or whatever, the tooltip code shouldn't need to change as where the spec or default comes from should be outside the concern of the Tooltip component itself. Btw. I might be off base as I'm new to the code, or there may be obstacles to such separation; or maybe such changes have already been planned, as it's a WIP PR

[monfera]

Likely a known issue: apparently randomly, storybook examples fail:

Cannot read property 'legendPosition' of undefined
TypeError: Cannot read property 'legendPosition' of undefined
    at http://localhost:9001/main.08acef345097e16e8e69.bundle.js:10032:57

then it may work on another try. Could be some race condition

[markov00]

There are a couple of places where some more uncoupling could be done, here's the main one: likely because of what chart types have been covered so far, Chart (src/components/chart.tsx) now depends on and builds an xy_chart (ie. stuff under src/chart_types/xy_chart) even when instantiating something like

You are right. as this is a WIP PR there still some refactoring that I need to go through before making it really available for review. Right not Chart depends on xy_chart for some event actions that needs to be moved into the XY chart process itself:

 this.chartStore.subscribe(() => {
      const state = this.chartStore.getState();
      const settings = getSettingsSpecSelector(state);
      if (this.state.legendPosition !== settings.legendPosition) {
        this.setState({
          legendPosition: settings.legendPosition,
        });
      }
      if (!isInitialized(state) || state.chartType !== ChartTypes.XYAxis) {
        return;
      }
      onElementOverCaller(state);
      onElementOutCaller(state);
      onElementClickCaller(state);
    });

IMHO I prefer to keep the main root component as Chart because I don't want to provide N multiple container classes that need to be changed depending on the chart type. When I will move the dependency into the XY chart there will no need to rename it in any other way.

[monfera]

Definitely not news to you but I was curious, all MobX dependencies can now be removed 🎉

yarn remove mobx mobx-react

PS. I guess you're keeping there until the last moment for when you switch back&forth between branches

[markov00]

@nickofthyme I've fixed the Annotation tooltips here: 143276c
I've left the shift+click for a future PR

[nickofthyme]

BAM!! 🎍

[nickofthyme]

YAAAAY!!!!

[markov00]

🎉 This PR is included in version 15.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀