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

[DISCUSS] State management plugin API #55977

Closed
lizozom opened this issue Jan 27, 2020 · 8 comments
Closed

[DISCUSS] State management plugin API #55977

lizozom opened this issue Jan 27, 2020 · 8 comments
Assignees
Labels

Comments

@lizozom
Copy link
Contributor

lizozom commented Jan 27, 2020

Assuming one is implementing a plugin and wants to use our state containers \ syncing utilities, what is the most minimal solution he could implement, in the demo plugin code below?

Assuming he wants filters, query and timefilter to be stored as in any other app (i.e. like dashboard) and also adding myName to app state and myGlobalData to global state

export const StateDemoApp = ({ basename, navigation, data }: StateDemoAppDeps) => {
  const [indexPattern, setIndexPattern] = useState<IIndexPattern>();
  const [filters, setFilters] = useState<esFilters.Filter[]>([]);
  const [query, setQuery] = useState<Query>({
    query: '',
    language: 'kql',
  });
  const [myName, setName] = useState<string>();
  const [myGlobalData, setGlobalData] = useState<string>();

  // store state here
  
  // sync state here
  
  return (
    <Router basename={basename}>
      <I18nProvider>
        <>
          <navigation.ui.TopNavMenu 
            appName="demo" 
            showSearchBar={true} 
            indexPatterns={[indexPattern]}
            query={query}
            filters={filters}
          />
          <EuiFieldText
            placeholder="My name is"
            value={myName}
            onChange={(e) => setName(e.target.value)}
            aria-label="My name"
          />
          <EuiHorizontalRule/>
          <EuiFieldText
            placeholder="My global data"
            value={myGlobalData}
            onChange={(e) => setName(e.target.value)}
            aria-label="My global data"
          />
        </>
      </I18nProvider>
    </Router>
@elasticmachine
Copy link
Contributor

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

@lizozom lizozom changed the title State management plugin API [DISCUSS] State management plugin API Jan 27, 2020
@Dosant
Copy link
Contributor

Dosant commented Jan 28, 2020

So here is how it could look with our current state containers and state syncing utils, which are already used in management and dashboard.

I also did some minor tweaks to state syncing, which made a lot of sense to me, while working on this demo.

Goal

Assuming one is implementing a plugin and wants to use our state containers \ syncing utilities, what is the most minimal solution he could implement, in the demo plugin code below?

Assuming he wants filters, query and timefilter to be stored as in any other app (i.e. like dashboard) and also adding myName to app state and myGlobalData to global state

So first of all let's define our state shape.

To make this work in current setup, we have to define 2 different state containers to store our state in, because technically we have different states which want to sync separately.

import { QueryAppState, QueryGlobalState } from '../../../../../src/plugins/data/public';

interface AppState extends QueryAppState {
  name: string;
}
const defaultAppState: AppState = {
  name: '',
};

interface GlobalState extends QueryGlobalState {
  globalData: string;
}
const defaultGlobalState: GlobalState = {
  globalData: '',
};

QueryAppState, QueryGlobalState are state shapes defined in data plugin:

  • In QueryGlobalState we will have global filters (pinned), time and refresh interval
  • In QueryAppState - app filters (not pinned filters)

Create state containers

Next we have to create state containers to our state.
It is done as simple as:

import { createStateContainer } from '../../../../../src/plugins/kibana_utils/public';

const appStateContainer = createStateContainer(defaultAppState);
const globalStateContainer = createStateContainer(defaultGlobalState);

But, if we don't want to worry about resetting the state when app gets unmounted, we should create it inside react component. And we don't want to recreate it on each render call. So it can be achieved by something like this:

function useCreateStateContainer<State extends BaseState>(
  defaultState: State
): ReduxLikeStateContainer<State> {
  const stateContainerRef = useRef<ReduxLikeStateContainer<State> | null>(null);
  if (!stateContainerRef.current) {
    stateContainerRef.current = createStateContainer(defaultState);
  }
  return stateContainerRef.current;
}

export const StateDemoApp = (props: StateDemoAppDeps) => {
  const appStateContainer = useCreateStateContainer(defaultAppState);
  const globalStateContainer = useCreateStateContainer(defaultGlobalState);

  return (
        <App {...props} />
  );
};

I think we should have this useCreateStateContainer helper in kibana_utils

Attach state containers to React.

There are state management helpers for connecting state containers to react components (look at it as on react-redux for redux).
These helpers trigger rerenders when state container get updates.

import {
  createStateContainerReactHelpers,
  ReduxLikeStateContainer,
} from '../../../../../src/plugins/kibana_utils/public';

const {
  Provider: AppStateContainerProvider,
  useState: useAppState,
  useContainer: useAppStateContainer,
} = createStateContainerReactHelpers<ReduxLikeStateContainer<AppState>>();

const {
  Provider: GlobalStateContainerProvider,
  useState: useGlobalState,
  useContainer: useGlobalStateContainer,
} = createStateContainerReactHelpers<ReduxLikeStateContainer<GlobalState>>();

export const StateDemoApp = (props: StateDemoAppDeps) => {
  const appStateContainer = useCreateStateContainer(defaultAppState);
  const globalStateContainer = useCreateStateContainer(defaultGlobalState);

  return (
    <AppStateContainerProvider value={appStateContainer}>
      <GlobalStateContainerProvider value={globalStateContainer}>
        <App {...props} />
      </GlobalStateContainerProvider>
    </AppStateContainerProvider>
  );
};

Now we have hook, we can use in child component to get access to underlying state containers.
useGlobalState, useGlobalStateContainer, useAppState, useAppStateContainer

Build the app container component using state containers

use state containers instead of React.useState for storing the state.
navigation.ui.TopNavMenu is state full and sync the UI with data.query services.
But we don't sync data.query services with our state containers at the moment.

const App = (props: StateDemoAppDeps) => {
  const appStateContainer = useAppStateContainer();
  const appState = useAppState();

  const globalStateContainer = useGlobalStateContainer();
  const globalState = useGlobalState();

  const indexPattern = useIndexPattern(data);
  if (!indexPattern) return <div>Loading...</div>;

  return (
       <>
          <navigation.ui.TopNavMenu
            appName={PLUGIN_ID}
            showSearchBar={true}
            indexPatterns={[indexPattern]}
          />
          <EuiFieldText
              value={appState.name}
              onChange={e => appStateContainer.set({ ...appState, name: e.target.value })}
            />
          <EuiFieldText
               value={globalState.globalData}
               onChange={e =>
                    globalStateContainer.set({ ...globalState, globalData: e.target.value })
               }
             />
        </>
  );
};

Setup syncing URL <-> State Containers <-> Data Services

data plugin in addition to QueryAppState, QueryGlobalState interfaces exposes helpers for connecting existing state containers to data services. connectToQueryGlobalState, connectToQueryAppState. These cover the syncing between State Containers <-> Data Services.
And then stateSync utility covers the syncing between url and state containers

function useGlobalStateSyncing<GlobalState extends QueryGlobalState>(
  globalStateContainer: BaseStateContainer<GlobalState>,
  query: QueryStart,
  kbnUrlStateStorage: IKbnUrlStateStorage
) {
  // setup sync state utils
  useEffect(() => {
    // sync global filters, time filters, refresh interval from data.query to state container
    const stopSyncingQueryGlobalStateWithStateContainer = connectToQueryGlobalState(
      query,
      globalStateContainer
    );

    // sets up syncing global state container with url
    const {
      start: startSyncingGlobalStateWithUrl,
      stop: stopSyncingGlobalStateWithUrl,
    } = syncState({
      storageKey: '_g',
      stateStorage: kbnUrlStateStorage,
      stateContainer: {
        ...globalStateContainer,
        // stateSync utils requires explicit handling of default state ("null")
        set: state => state && globalStateContainer.set(state),
      },
    });

    // skipped here!
   // Manually take care of initial syncing. This is app specific!
   // e.g. some initial state could come from saved object and it should take precedence
   // e.g.  or there could be custom options like "Save time with Dashboard" which will change this  behaviour per app

    // finally start syncing state containers with url
    startSyncingGlobalStateWithUrl();

    return () => {
      stopSyncingQueryGlobalStateWithStateContainer();
      stopSyncingGlobalStateWithUrl();
    };
  }, [query, kbnUrlStateStorage, globalStateContainer]);
}

function useAppStateSyncing<AppState extends QueryAppState>(
  appStateContainer: BaseStateContainer<AppState>,
  query: QueryStart,
  kbnUrlStateStorage: IKbnUrlStateStorage
) {
  // setup sync state utils
  useEffect(() => {
    // sync app filters with app state container from data.query to state container
    const stopSyncingQueryAppStateWithStateContainer = connectToQueryAppState(
      query,
      appStateContainer
    );

    // sets up syncing app state container with url
    const { start: startSyncingAppStateWithUrl, stop: stopSyncingAppStateWithUrl } = syncState({
      storageKey: '_a',
      stateStorage: kbnUrlStateStorage,
      stateContainer: {
        ...appStateContainer,
        // stateSync utils requires explicit handling of default state ("null")
        set: state => state && appStateContainer.set(state),
      },
    });


   // skipped here!
   // Manually take care of initial syncing. This is app specific!
   // e.g. some initial state could come from saved object and it should take precedence
   // e.g.  or there could be custom options like "Save time with Dashboard" which will change this  behaviour per app

    // finally start syncing state containers with url
    startSyncingAppStateWithUrl();

    return () => {
      stopSyncingQueryAppStateWithStateContainer();
      stopSyncingAppStateWithUrl();
    };
  }, [query, kbnUrlStateStorage, appStateContainer]);
}

And then in the app:

const App = ({
  data,
  kbnUrlStateStorage,
}: StateDemoAppDeps) => {
  const appStateContainer = useAppStateContainer();
  const appState = useAppState();

  const globalStateContainer = useGlobalStateContainer();
  const globalState = useGlobalState();

  useGlobalStateSyncing(globalStateContainer, data.query, kbnUrlStateStorage);
  useAppStateSyncing(appStateContainer, data.query, kbnUrlStateStorage);

  /// render ui
}

Complete example:

https://github.com/elastic/kibana/pull/56128/files#diff-a0e48df3e83e86c617c169f43d50072e

Some points to note:

  1. The flow follows this original proposal [discuss] Global state & URLs in the New Platform #39855

  2. connectToGlobalState connectToAppState are just helpers to replicate how syncing works in current Kibana with _a and _g. Apps can decide to not use these and build there own syncing on top of state_management primitives kibana_utils exposes or not do any syncing at all (just use data.query services directly instead).

  3. If not for the requirement in description to extend _g state with custom stuff (globalData) then setting up syncing of _g portion could be a lot simpler. As all of the data in _g would come from data services, we could use this higher level helper:

import { syncGlobalQueryStateWithUrl } from `../src/plugins/data/public`
syncGlobalQueryStateWithUrl(data.query, kbnUrlStateStorage)

(this is used by dashboard to sync all _g part of url

  1. To use connectToGlobalState connectToAppState or syncState it is not required to use createStateContainer. The input argument to those helpers should just implement simple interface:
export interface BaseStateContainer<State extends BaseState> {
  get: () => RecursiveReadonly<State>;
  set: (state: State) => void;
  state$: Observable<RecursiveReadonly<State>>;
}

e.g in dashboard app filters are synced with custom state manager by just:

const stopSyncingAppFilters = connectToQueryAppState(queryService, {
      set: ({ filters }) => dashboardStateManager.setFilters(filters || []),
      get: () => ({ filters: dashboardStateManager.appState.filters }),
      state$: dashboardStateManager.appState$.pipe(map(state => ({ filters: state.filters }))),
    });

@lukeelmers
Copy link
Member

I think we should have this useCreateStateContainer helper in kibana_utils

This seems like a good idea to me, assuming we document the specific reasons why you might want it.

If not for the requirement in description to extend _g state with custom stuff (globalData) then setting up syncing of _g portion could be a lot simpler.

IMO all of the extra work to get _a and _g working is a necessary evil in order to make things backwards compatible. However, for folks building new features/apps who want to do state syncing, my recommendation would be to avoid merging their app state into the same key as shared state from other plugins (as we do with _g). There's no need to do it other than for aesthetic reasons or backwards compatibility.

After all, there is no more truly "global" state anymore, just state shared between apps. Just because some state is pushed into a _g param in the URL doesn't mean it becomes available globally like it does in the legacy world.

Assuming he wants filters, query and timefilter to be stored as in any other app (i.e. like dashboard)

This is what I think we should steer folks away from -- the way URLs are structured in the dashboard is not how IMO they should look long term. So if someone wants to sync filters, query, timefilter to their app URL, plus some other stuff specific to their app, I think the minimal implementation would be even simpler if they had filters, query, timefilter, myName, and myGlobalData all as separate URL params. (@Dosant does this sound right to you?)

All this is to say, in the example in this thread, the requirements for having a dashboard-like URL is where a lot of the complexity comes from.

@Dosant
Copy link
Contributor

Dosant commented Jan 31, 2020

I think the minimal implementation would be even simpler if they had filters, query, timefilter, myName, and myGlobalData all as separate URL params. (@Dosant does this sound right to you?)

@lukeelmers, Yes, this was also my understanding. Also for more context with new utilities, please review - #56479 . Maybe some new thoughts will appear from there.

@Dosant
Copy link
Contributor

Dosant commented Jan 31, 2020

@streamich, Could you take a look please link. Interested in your thoughts since you've been working a lot on state containers.

While I was working on the example, had couple thoughts on improvements:

  1. Please see my useCreateStateContainer helper. It is simplified version without transitions and selectors. I wonder if it think it makes sense to implement one in kibana_utils? What you you think?

  2. For this simple example I didn't use transitions or selectors. I just used set() and get(). I was getting the state by useState() hook, but to be able to set state I had to also pull useStateContainer.
    What do you think of changing useState to be the same as React.useState. This way I wouldn't have to use separate useStateContainer hook at all in my example and the api would look closer to react's one.

const [state, setState] = useState() 

Wonder what do you think about those 2?

@Dosant
Copy link
Contributor

Dosant commented Feb 3, 2020

We should also consider this issue: #56503

In the #55818 we have to "watch" for any update coming from either global filters, time range or refresh interval and then update _g urls in navbar with that information.

In legacy it was done in one place in chrome and chrome updated all the urls in navbar. But in np, it was decided that apps "watch" for those updates themselves and update their own url in navbar.

With current setup as in #55818, an app creates a state container which orchestrates changes from "global filters, time range or refresh interval". And each app have to orchestrate it. These orchestration is happening when apps are not mounted (see the setup phase of dashboard plugin).
The orchestration is happening with similar to connectToQueryGlobalState from this example #55977 (comment)

It looks like we need to make sure we have observables on data.query like:

globalStateQueryChange$
appStateQueryChange$
queryChange$ 

which could be used for cases like in #55818

@streamich
Copy link
Contributor

I'm not sure I fully understand details here, but if I was writing an app I would try to use a single state container, similar how you have a single Redux store per app, as state containers are essentially Redux stores.

Something like this:

interface DashboardState {
  globalStuff: any;
  isEditMode: boolean;
  // etc.
}

const dashboardContainer = createStateContainer<DashboardState>();
const {useSelector, Provider} = createReactStateContainerHelpers(dashboardContainer);

<Provider>
  <DashboardApp />
</Provider>

const DashboardApp = () => {
  const { isEditMode } = useSelector(({isEditMode}) => ({isEditMode}));

  return <div>this is dashboard</div>;
};

@Dosant
Copy link
Contributor

Dosant commented Feb 27, 2020

Short term resolved by #56128

@Dosant Dosant closed this as completed Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants