Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(refactor): refactor chart specific selectors out of state #2613

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Feb 17, 2025

Summary

Refactors chart specific selectors out of redux state.

Details

The selectors and renderer in internalChartState are functions which means they are not suitable to be part of redux state. This PR removes internalChartState from the redux state. An unfortunate effect of doing this is that it causes quite some circular dependency problems because of two-way imports between the main chart_state code and chart specific code. To solve this, the following changes have been done:

  • Originally internalChartState included both a renderer and selectors. Part of the circular dependency problems was that the renderer included React components, which use the selectors, which make use of getInternalChartState, which again includes the renderer!
  • This PR splits renderer and selectors into different factories:
    • A chartRenderer will now only be called from chart_container.tsx so it's no longer imported anywhere from the main chart_state.
    • chartSelectors use mostly the same interface as internalChartState previously. However, instead of importing all code backwards from individual charts back to main chart_state, in chart_state we now have just a typed chart selector registry. That means we are no longer importing selectors code from individual charts back to main chart_state. Instead, in components/chart.tsx the registry is populated with selectors from individual charts.
    • Previously InternalChartState implemented a class. It is now refactored into a factory that returns default implementations which can be overridden by individual charts. This means the defaults are now part of main chart_state and don't need to be replicated by every individual chart. This more or less flips around the implementation of defaults which should reduce some circular dependency risk too. It also reduces the risk of diverging default implementations.

Issues

Part of #2078 and #2615.

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)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with light and dark themes

@walterra walterra self-assigned this Feb 17, 2025
@walterra walterra force-pushed the redux-internal-selectors branch from cbeb5cd to c31b452 Compare February 17, 2025 15:23
@walterra walterra force-pushed the redux-internal-selectors branch from 0328248 to d03b5fc Compare February 21, 2025 13:48
@walterra walterra added the chore label Mar 4, 2025
@walterra walterra force-pushed the redux-internal-selectors branch from d477177 to db0af57 Compare March 4, 2025 17:08
@elastic-datavis
Copy link
Contributor

✅ Successful Deployment (build#4595) - b875fde

@walterra walterra marked this pull request as ready for review March 4, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant