[POC] Application state syncing utility.

[lukeelmers]

[skip-ci]

Do not merge

Goals

This is a POC for an app & global state syncing utility as discussed in #44151. Please review that issue for a high-level overview of what's happening here.

The ultimate goal here is to provide a solution that will replace the legacy ui/state_management -- particularly AppState and GlobalState. This POC exists to show how such a solution might look, and to gather feedback on the design as we move toward implementation.

The next steps from here will be creating the utilities in the agreed-upon design with proper tests, etc, after which we can begin gradually cutting applications over to this new way of handling state.

Description

There is a lot to look at here, but there are really just two main areas to focus on which will comprise the public API:

1. The syncState utility itself. This is where most of the magic actually happens.
2. URL utilities. These handle rison encoding/decoding, as well as URL hashing/unhashing. Most users will not need to use these directly, as syncState will talk to them internally.

We think this is not far from the final solution, but there is still some caveats and edge cases we'd like to figure out before moving forward.

Currently, it does the following things:

• It knows how to take a state container (IStore) and subscribe to its changes (or watch only for change in a specific slice)
• It can persist in the state using SyncStrategy object. There are 2 already implemented SyncStrategies
  • URL (Replicates what AppState / GlobalState did with Rison serialization)
  • HashedURL (Replicates what AppState / GlobalState with config:syncStateToSessionStorage tune on
  • It is possible to provide own custom sync strategies by just implementing object matching ISyncStrategy interface: https://github.com/elastic/kibana/pull/51692/files#diff-905a361fe3953ab31914d5421363348dR171
• SyncStrategy also responsible for notifying about storage updates, sync state utility also triggers applicate state updates when storage has changed

Demo

We integrated the changes with the smallest use case of AppState we could find: the index patterns edit page. You can test by going to index patterns, clicking edit on a pattern, and toggling between tabs, typing filter, changing index pattern type in select. The changes to the URL are happening via the state sync utility. The UI updates are happening via a local state container that the controller is subscribed to. Browser history and page reload update UI to the corresponding state.

Please check out this demonstration:
https://drive.google.com/open?id=15sJp1M8mHBrkd0ClE1QiunxByeMfbJnz
Just for a demo, it is configured as 'tab' is always encoded as Rison in the URL,
But other state slices respect state:storeInSessionStorage config flag.
The configuration itself:
https://github.com/elastic/kibana/pull/51692/files#diff-e01c180041e1a85b1baec38ca1446211R220

Usage examples

Minimal usage example:

 type State = {tab: string};
 const store: IStore<State> = createStore({tab: 'indexedFields'})

 syncState({
   syncKey: '_s',
   store: store
 })

Now State will be synced with URL:

• URL will be updated on any store change
• the store will be updated on any URL change

By default SyncStrategy.Url is used, which serializes state in rison format

The same example with different sync strategy depending on kibana config:

syncState({
  syncKey: '_s',
  store: store,
   syncStrategy: config.get('state:storeInSessionStorage') ? SyncStrategy.HashedUrl : SyncStrategy.Url
})

If there are multiple state containers:

type State1 = {tab: string};
const store1: IStore<State> = createStore({tab: 'indexedFields'})

type State2 = {filter: string};
const store2: IStore<State> = createStore({filter: 'filter1'})

syncState([
 {
   syncKey: '_g',
   store: store1
 },
 {
   syncKey: '_a',
   store: store2
 }
])

It is possible to provide custom synching strategy.
Something very simple for local storage will look like this:

const localStorageSyncStrategy = {
         toStorage: (syncKey, state) => localStorage.setItem(syncKey, JSON.stringify(state)),
         fromStorage: (syncKey) => localStorage.getItem(syncKey) ? JSON.parse(localStorage.getItem(syncKey)) : null
       };
syncState({
     syncKey: '_s',
     store,
     syncStrategy: localStorageSyncStrategy
});

syncState returns destroy function

const destroy = syncState();
destroy(); // stops listening for state and storage updates

Caveats & Open Questions

Batching url updates issue

We decided to schedule all url updates to the next micro task.
This allows to do multiple synchronous store updates and don't cause intermediate history steps.
e.g.:

 type State = {tab: string, filter: string};
 const store: IStore<State> = createStore({tab: 'indexedFields', filter: 'filter1'})

 syncState({
   syncKey: '_s',
   store: store
 })


function resetButtonClicked() {
 resetTab();
 resetFilters();
}

function resetTab() {
 store.dispatch(resetTabAction())
}

function resetFilters() {
 store.dispatch(resetFiltersAction())
}

When a user clicks on a "Reset" button the click is handled by resetButtonClicked handler.
Unfortunatly, it is implemented in a way, that it causes 2 state updates.
But with our current sync state util implementation, it will make URL to update only once, because url updates are scheduled to the next micro task, which allows to batch multiple updates.

As this problem is URL specific (because of history), not all storage strategies will have this issue. So we solved it on URLStrategy level.

[joshdover]

How valuable is the toStorageWrapper / fromStorageWrapper feature? For example, one could do this instead:

type State = {tab: string, filter: string};
const store: IStore<State> = createStore({tab: 'indexedFields', filter: 'filter1'})

const mapStateToTab = state => state.tab;

syncState({
  syncKey: '_s',
  store: store,
  store: {
    get: () => mapStateToTab(store.get()),
    set: state => store.set({ ...store.get(), tab: state }),
    state$: store.state$.pipe(map(mapStateToTab)),
  },
})

It's slightly more boilerplate, but I think it's more clear what is happening and requires consumers to learn less about the syncState API.

In addition, removing this feature would be one less thing to maintain and test.

[flash1293]

There is a related topic which is currently on the Kibana App roadmap and I wonder how it ties into this work: In src/legacy/ui/public/chrome/api/nav.ts there is some functionality around storing the current url for a given app (including state). This code currently knows about the url state (see injectNewGlobalState function) and parts of the state stored like this is shared among apps.

It looks like this should live close to the syncState helper. Has there been thoughts around this already?

[Dosant]

@joshdover, those mappers just seemed to me like an api which would simplify a bit a bunch of very common use cases.

I gave it a try and removed those mappers: lukeelmers#2 .
Looking at diff, I am not sure, that it is better without them. Feels to me like this made consumer code more complicated, than helper code less complicated.

So not sure.

Maybe I am wrong and those use cases are not so common, as I think (except backward compatibility), so then indeed I'd remove those apis.

[joshdover]

Looking at diff, I am not sure, that it is better without them. Feels to me like this made consumer code more complicated, than helper code less complicated.

To me it looks like it just added 2-3 lines of mostly braces and parentheses to the consumer code?

I just think having a single source of truth of the state in the stateSync code is going to avoid a bug in the future. I can easily see a case where we add a feature to this and forget to pipe the store through the storage mapper and it breaks down the line.

Up to you though, I won't be the one maintaining this 😄

[joshdover]

One way to think about this is: how much cognitive load does each option create?

I think the amount of cognitive load added to syncState by including the toStorageMapper is greater than the amount put on consumer.

As library code, syncState needs to be rock solid. Any bugs in this code are going to create some really annoying UX bugs for users. It's already a non-trivial amount of behavior (500 lines w/o tests) and having more than one source of truth significantly raises that cognitive load on the developer as they make changes to this.

On the other hand, each plugin developer only has to write ~10-20 lines to use syncState, in the most advanced usecases. This is easier to reason about since it's such a small amount of logic.

We've also discussed how developers may eventually use an undo/redo mechanism in front of syncState to only sync some of the changes to storage. In that case, the developer is already going to be wrapping their store with some sort of filtering mechanism to dictate which changes to persist. Why not use the same wrapper pattern for mapping the store to the state schema?

Regardless, this is definitely not the hill I'm going to die on, and I think this is a pretty minor case, just my 2¢. Either way you go will work.

[lukeelmers]

removing this feature would be one less thing to maintain and test.

One way to think about this is: how much cognitive load does each option create?

These are both great points and I tend to agree -- my vote would be to remove the mappers and instead let apps handle this on their own. The least risky approach here is to introduce an API with the smallest possible surface area. And if we see a great pattern emerge for apps to handle these mappings, there's no reason we couldn't revisit it later.

Plus, as @Dosant rightfully points out, one of the main reasons for adding this today is backwards compatibility. Time will tell if providing a unified solution for mappers is truly necessary, so if we can accomplish the same thing outside of syncState, it's even better.

In src/legacy/ui/public/chrome/api/nav.ts there is some functionality around storing the current url for a given app (including state). This code currently knows about the url state (see injectNewGlobalState function) and parts of the state stored like this is shared among apps.

It looks like this should live close to the syncState helper. Has there been thoughts around this already?

So @joshdover may be able to confirm the plans around this particular chrome API, but generally speaking the intent here is to not have any state stored in one place and shared among apps. What syncState will allow us to do is completely decouple apps from the global state, instead preferring individual apps to subscribe to state updates from the other apps they care about. Then whether state is stored in the URL becomes an implementation detail of that app.

Not sure if this is exactly what you were asking @flash1293 -- does that answer your question?

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[joshdover]

@lukeelmers Is this ready for code review?

[lukeelmers]

@joshdover Yes it is -- I will mark "ready for review" although we do not expect to merge the PR in its current form (the integration with the Edit Index Patterns page was for demo purposes only, so we won't merge as-is without making updates there).

[lukeelmers]

The goal here is really to get alignment on the actual design. Before we start integrating anything "real" with it, we'll need to go back and add tests and so forth to be sure it's ready.

[flash1293]

Not sure if this is exactly what you were asking @flash1293 -- does that answer your question?

Partially yes. I agree that it makes sense to make this handling of the respective app without a shared service but there will still be several apps (discover, visualize, dashboard, lens?) that work in the same way regarding the state management and thus would profit from a shared stateless utility function that implements the behavior. I kind of imagine it as a layer on top of the pure syncState that offers API along the lines of saveAppSubUrl(appId) (called on app unmount) and restoreAppSubUrl(appId) (call on app mount) handling the details like synchronizing the filter and timerange state of data in the right order and so on.

Of course each app could implement the logic itself but it looks like the behavior of several apps is similar enough to justify a shared helper function. That would also make it easier to have the same behavior in other apps (like Graph or Maps) to make the Kibana experience more consistent in the long run.

But on a second look that's only tangentially related to the things in this PR so we should probably discuss it somewhere else.

[flash1293]

I tried to think through the integration of this into dashboard and right now I can't think of any blockers, but it would probably require a few additional things on top of it.

I have no idea whether it makes sense to push this stuff down into the state syncing utility itself, so I think it makes sense to start with these primitives (that definitely make sense for a lot of cases, great work there!) and see whether enough similar integrations emerge that justify a consolidation.

[stacey-gammon]

👍 I like it!

[Dosant]

@joshdover, @lukeelmers, I removed mappers 👍

[lukeelmers]

We are planning to close this POC PR shortly as we begin moving forward with implementation.

If anybody has remaining comments on the design, now is the time to bring them up 😄

Otherwise we will close this by end of week.

[Dosant]

Implementation is ready for review #53582:

It covers advanced edge case scenarios:

1. Replicate Global State _g using state syncing utility
2. Preserve app state during navigation between different apps (kinda src/legacy/ui/public/chrome/api/nav.ts) [POC] Application state syncing utility. #51692 (comment) cc @flash1293
3. Working together with react-router

It doesn't look like we are missing something critical with current approach, but please take a look if that sounds good.

[Dosant]

#53582