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

State Management: merge state in one place #6219

Open
ericpgreen2 opened this issue Dec 5, 2024 · 1 comment
Open

State Management: merge state in one place #6219

ericpgreen2 opened this issue Dec 5, 2024 · 1 comment
Assignees

Comments

@ericpgreen2
Copy link
Contributor

ericpgreen2 commented Dec 5, 2024

Follow-up to #5675

One reason our state management is complex is because we have:
a) Many sources of state (which is unavoidable, per our requirements)
b) State merging happening in multiple, disconnected places. We have a waterfall of state merges.

To simplify, I’d propose we merge the state together in one canonical place, instead of the current waterfall approach.

I count these sources of state (in increasing order of priority):

  • Rill-opinionated default
  • developer-specified preset
  • user time zone preference
  • home bookmark
  • magic token state
  • local storage ("last viewed state")
  • session storage
  • URL query parameters

Some discrete issues:

  • Multiple, disconnected state merges: getDefaultExplorePreset() does a subset of state merge: it merges the Rill-opinionated default, the developer-specified default, and the user's time zone preference . Then convertURLToExplorePreset() does another subset of state merge: it merges the defaultExplorePreset with URL parameters. These functions encompass two state merges, yet at some other point we also merge state from bookmarks, local storage, and session storage. It's hard to track.
  • Conversions into ExplorePreset: We should not be using the ExplorePreset type as a type for intermediate state handoffs. convertSourceXToExplorePreset() shouldn’t be necessary. We should only have to convert state sources into Partial<ExploreState>.

Suggestions:

  • Replace convertSourceXToExplorePreset(), getExplorePresetForSourceX() functions with getPartialStateFromSourceX() functions. Colocate these functions.
  • Add one canonical merge, e.g. const state = mergeState(sourceXPartialState, sourceYPartialState, ...)
  • In the merge function, clearly list sources in priority order from lowest (Rill defaults) to highest (URL params)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants