Reduce risk of circular dependencies.

[walterra]

At the moment, updating/refactoring the elastic-charts code base includes the risk of easily introducing circular dependencies. Resolving these can be time consuming and it makes PRs larger than necessary because of the required additional refactorings.

This issue tries to summarize approaches that can be used to reduce the risk of running into circular dependencies.

What is causing the risk?

The code base is organized in more general "base" code under packages/charts/src/state and specific code for different chart types in packages/charts/src/chart_types. Just as an analogy (I'm not suggesting refactoring anything towards being more OOP), in classic OOP we could consider the state code the classes and chart_types the implementations or instanced classes. The important bit is that the relationship is unidirectional state -> chart_types. The current code base handles this at just a very informal level and there are no restrictions in place to avoid two-way-dependencies. One of the main culprits is that what's called internal state at the moment is reimporting everything from chart_types to set up the chart specific renderers and selectors.

Analysis with dependency-cruiser

Once you start refactoring and run into a circular dependency it's hard to find the actual root cause. dependency-cruiser is a library to help with this and can point you to the exact files causing the problems, for example:

warn no-circular: 
      packages/charts/src/chart_types/heatmap/state/chart_state.ts →
      packages/charts/src/chart_types/heatmap/state/selectors/compute_chart_dimensions.ts →
      packages/charts/src/chart_types/heatmap/state/selectors/compute_axes_sizes.ts →
      packages/charts/src/state/selectors/compute_small_multiple_scales.ts →
      packages/charts/src/state/selectors/get_internal_main_projection_area.ts →
      packages/charts/src/state/selectors/get_internal_chart_state.ts →
      packages/charts/src/chart_types/heatmap/state/chart_state.ts

On top of this out-of-the-box-feature, one can describe custom rules for the validation. For example, we might want to warn or error out when someone tries to import from chart_types into state:

{
      name: 'two-way-chart-type-not-allowed',
      severity: 'error',
      from: { path: '^packages/charts/src/state' },
      to: { path: '^packages/charts/src/chart_types' },
}

$ npx depcruise packages/charts/src
...
error two-way-chart-type-not-allowed: packages/charts/src/state/utils.test.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/spec_factory.test.tsx → packages/charts/src/chart_types/xy_chart/specs/bar_series.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/is_external_tooltip_visible.ts → packages/charts/src/chart_types/xy_chart/state/selectors/get_computed_scales.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/get_tooltip_spec.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/get_small_multiples_spec.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/get_small_multiples_index_order.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/get_settings_spec.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/can_pin_tooltip.ts → packages/charts/src/chart_types/xy_chart/state/selectors/get_tooltip_values_highlighted_geoms.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/selectors/can_pin_tooltip.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/reducers/interactions.ts → packages/charts/src/chart_types/partition_chart/state/selectors/picked_shapes.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/reducers/interactions.ts → packages/charts/src/chart_types/partition_chart/state/selectors/drilldown_active.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/reducers/interactions.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/xy_chart/state/chart_state.tsx
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/wordcloud/state/chart_state.tsx
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/timeslip/internal_chart_state.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/partition_chart/state/chart_state.tsx
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/metric/state/chart_state.tsx
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/index.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/heatmap/state/chart_state.tsx
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/goal_chart/state/chart_state.tsx
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/flame_chart/internal_chart_state.ts
  error two-way-chart-type-not-allowed: packages/charts/src/state/chart_state.ts → packages/charts/src/chart_types/bullet_graph/chart_state.tsx

Besides validation and analysis problems at hand, dependency-cruiser can generate charts that visualize the dependency graph:

Note that this includes the custom rule we defined and highlights import from chart_types to state in red!

Note that the rules could also be defined using eslint.

linting imports/exports

Barrel files can be problematic in relation to circular dependencies too. (Background: https://marvinh.dev/blog/speeding-up-javascript-ecosystem-part-7/)

export * from "./foo";
export * from "./bar";
export * from "./baz";
// stuff like this is esp. tricky:
export * from "../some-other-dir-further-up";

linting type imports

This could improve readability and avoid some issues with imports. In Kibana some plugins enforce linting to have import type { .

refactor internal chart state

To avoid circular dependencies between state and chart_types, we can refactor InternalChartState:

• break out the renderers to be only imported from chart/chart_container.tsx
• break out the selectors into a dynamic registry. getInternalChartState would only define the registry, when setting up the store we can initialize the registry.

task summary

Potential tasks to improve code base related to circular dependencies:

• Set up dependency-cruiser as part of the repo.
  chore(deps): dependency cruiser #2616
  • use for validation
  • use for visualization
• Set up linting rules to avoid barrel files
  • Evaluate https://www.npmjs.com/package/eslint-plugin-barrel-files
• Set up linting to enforce usage of import type ...
  chore(types): consistent type imports #2618
• refactor internal chart state
  chore(refactor): refactor chart specific selectors out of state #2613