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] State syncing utilities #53582

Merged
merged 66 commits into from
Jan 10, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Dec 19, 2019

Summary

Today, apps rely on AppState and GlobalState in the ui/state_management module to deal with internal (app) and shared (global) state. These classes give apps an ability to read/write state, when is then synced to the URL as well as sessionStorage. They also react to changes in the URL and automatically update state & emit events when changes occur.

This PR introduces new state synching utilities, which together with state containers src/plugins/kibana_utils/public/state_containers will be a replacement for AppState and GlobalState in New Platform.

This PR is part of #44151.
This PR is implementation of #51692 POC, but also covers a bunch of advances use cases such as:

  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)
  3. Working together with react-router. Makes sure it can work with react router in both modes: history and browser

Example App

There are 2 todo apps with different configs. React routers have different setup (browser and hash history). syncState util have different setup (different syncKeys)
The todo state is synched with different keys, so the state is not shared between apps.
The '_g' state is using the same key, so it is shared between apps.
video: https://drive.google.com/open?id=1q4RH75u59FhMn83d4a2Q5woI4TbiMVu3

To run with example todo apps:
yarn start --run-examples

Screenshot 2019-12-19 at 16 09 13

Example covers this advanced cases:

Replicate Global State (_g) using state syncing utility

To replicate GlobalState we just can define a common state container as static utility, use it in multiple apps and setup syncState utility in the same way in all the apps we need it (important to use the same storage keys and state storages).

Preserve app state during navigation between different apps

If all the app state, that we need to preserve is located inside state containers, then we can just normally use syncState utility with synching state to sessionStorage. This state will be restored from sessionStorage when navigated back to the app.

But looking at src/legacy/ui/public/chrome/api/nav.ts, it is clear that not all the state is in the state containers. Some of important pieces could be part or routing. e.g. /edit/:id/#?_g= - here edit/:id/ won't be part of state container.

To cover this scenario - separate tiny urlTracker util was introduced and it is used in example app to track state from react-router routes which are not part of state container.

Caveats:

  • this implementation uses history, which probably can't be integrated with angular? so will have to wire it up to angular manually using 'trackUrl' method.
  • src/legacy/ui/public/chrome/api/nav.ts is part of chrome and it changes the app links in sidebar. As our utility would be decoupled from chrome, the links to apps will be static. The url will be restored by the application itself. [State Management] State syncing utilities #53582 (comment).

React Router

To make syncState utils work seamlessly with react-router, had to make sure that they use the same history instance. Hope it won't be needed in future, as apparently in v5 version history will be a singleton - https://github.com/ReactTraining/history/releases/tag/v5.0.0-beta.2

Also makes sure it can work with react router in both modes: history and browser.

I also opened issues to platform:

Other Example

Dev Docs

#43511

@Dosant Dosant marked this pull request as ready for review December 19, 2019 13:01
@Dosant Dosant requested a review from a team as a code owner December 19, 2019 13:01
@Dosant Dosant self-assigned this Dec 19, 2019
@elasticmachine
Copy link
Contributor

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

…agment/state-sync

# Conflicts:
#	package.json
@joshdover
Copy link
Contributor

One use case that may be broken here:

Opening a navlink from the Chrome with Cmd+Click will not open to the same location since the sessionStorage will be a fresh instance in the new tab.

At one point in time, I thought this could be solved by using localStorage instead, but if I remember correctly, using localStorage caused tabs to override each other's last state when clicking between apps.

One way to solve this would be to actually update the Chrome navLink URLs, which there is an API for: chrome.navLinks.update. This allows "bootstrapping" a new tab with the last state of the app while keeping each tab in separate sessions. Though, we should (1) see if this behavior is actually desired; and (2) explore alternative ways of doing this, this is just one way that we currently do this.

@Dosant
Copy link
Contributor Author

Dosant commented Dec 20, 2019

@joshdover, I just looked into chrome.navLinks.update and apparently, right now, by design, core doesn't support updating urls in nav for np apps. I guess the assumption was, that all state restoration will be handled by the app itself.

https://github.com/elastic/kibana/blob/master/src/core/public/chrome/nav_links/nav_link.ts#L88
https://github.com/elastic/kibana/blob/master/src/core/public/chrome/ui/header/header.tsx#L112

I am not sure if there any other way to update that url in nav, but if we decide that we want to leave that behaviour, likely we will indeed will have to support that update for np apps as well..

Theoretically, I liked the idea of having all state restoration responsibilities on app itself. Less code in core and less dependencies on core.

we should (1) see if this behavior is actually desired

Just my opinion as a user, as I was digging into that url preservation behaviour yesterday for the first time: I was surprised that clicking on a link of currently active app or opening it in a new tab doesn't restore the state to the default state. I think, as I user, I am used to that clicking on a "home" button will get me do default state, and, for some reason, I expected to see that happening for the active app button.
So IMHO, for me that it is not desired :)

@flash1293
Copy link
Contributor

Just my opinion as a user, as I was digging into that url preservation behaviour yesterday for the first time: I was surprised that clicking on a link of currently active app or opening it in a new tab doesn't restore the state to the default state. I think, as I user, I am used to that clicking on a "home" button will get me do default state, and, for some reason, I expected to see that happening for the active app button.

Totally agree for the new tab / clicking the active app icon scenario, we should just make sure to keep the current behavior when switching between apps in the same tab. It's also a slight enough behavior change that at least IMHO we don't have to wait till 8.0 to make it.

@Dosant
Copy link
Contributor Author

Dosant commented Dec 20, 2019

@joshdover, I bumped into this issue #53692 which is related to sidenav navigation.
Created different issue, as I think it is more related to platform, then to utils I am working on here.

@Dosant
Copy link
Contributor Author

Dosant commented Jan 8, 2020

Per discussion with @lizozom:
renamed SyncStrategy -> StateStorage and nicely simplified it's interface.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits, but I didn't get through everything. LGTM so far. I will continue on Friday, so don't block merging on me.

import rison, { RisonValue } from 'rison-node';
import { isStateHash, retrieveState, persistState } from '../state_hash';

export function decodeState<State>(expandedOrHashedState: string): State {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense in this file to restrict the State type param to RisonValue?

export function decodeState<State extends RisonValue>(expandedOrHashedState: string): State {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was trying to make it nicer for a while.

Bumped into problem, that if I start to specifically expect RisonValue here, then the whole type chain up to BaseStateContainer breaks, as in state_containers we don't have any restrictions on State shape.

So likely the best approach would be to reconsider if we want to have proper restrictions on base state shape which would satisfy RisonValue.

I will address this separately later.

@@ -0,0 +1,246 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Why are all of these prefixed with Kbn? I don't have strong feeling about this, but it looks like we could just call it url_storage. Maybe I'm missing something here.

Copy link
Contributor Author

@Dosant Dosant Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially it was without Kbn prefix, but I reconsidered and added Kbn just to emphasise this is type of storage is something which is coming from legacy Kibana and Kibana App.

I thought that later we could have differently named url storage, which would allow to preserve state in nicer and simpler user friendly format (e.g by serialising using regular query params like &query="test"&time="test&filter="filter1,filter2"), where Kbn is something took from old Kibana, with rison, storing in hash part of url and other details.

@stacey-gammon
Copy link
Contributor

Looked through the code, pulled down and was able to add url tracking to the embeddable explorer. I would like to keep playing around with it to get a better feel, but the direction looks solid, and I think we can iterate as we use it in more real world use cases (like dashboard).

Great example plugins! LGTM.

@Dosant Dosant force-pushed the dev/state-managment/state-sync branch from 1ae6f70 to b8d808c Compare January 10, 2020 12:41
@Dosant Dosant merged commit a7a557b into elastic:master Jan 10, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request Jan 10, 2020
Today, apps rely on AppState and GlobalState in the ui/state_management module to deal with internal (app) and shared (global) state. These classes give apps an ability to read/write state, when is then synced to the URL as well as sessionStorage. They also react to changes in the URL and automatically update state & emit events when changes occur.

This PR introduces new state synching utilities, which together with state containers src/plugins/kibana_utils/public/state_containers will be a replacement for AppState and GlobalState in New Platform.
@Dosant Dosant deleted the dev/state-managment/state-sync branch January 13, 2020 05:48
Dosant added a commit that referenced this pull request Jan 13, 2020
Today, apps rely on AppState and GlobalState in the ui/state_management module to deal with internal (app) and shared (global) state. These classes give apps an ability to read/write state, when is then synced to the URL as well as sessionStorage. They also react to changes in the URL and automatically update state & emit events when changes occur.

This PR introduces new state synching utilities, which together with state containers src/plugins/kibana_utils/public/state_containers will be a replacement for AppState and GlobalState in New Platform.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Dosant added a commit that referenced this pull request Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in #53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting #54438
Transitions and Selectors allow any state, not bind to container's state #54439
Dosant added a commit to Dosant/kibana that referenced this pull request Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in elastic#53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting elastic#54438
Transitions and Selectors allow any state, not bind to container's state elastic#54439
@Dosant Dosant added v7.6.0 and removed v7.7.0 labels Jan 13, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Dosant added a commit that referenced this pull request Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in #53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting #54438
Transitions and Selectors allow any state, not bind to container's state #54439
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in elastic#53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting elastic#54438
Transitions and Selectors allow any state, not bind to container's state elastic#54439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants