[State Management] State containers improvements #54436

Dosant · 2020-01-10T10:56:24Z

Summary

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 I noticed, but didn't fix in reasonable time:

Can't use addMiddleware without explicit type casting [State Management] State containers and redux middlewares types mismatch #54438
Transitions and Selectors allow any state, not bind to container's state [State Management] Transitions and selectors should know about state shape #54439

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…agement/state-containers-improvements

Dosant · 2020-01-10T10:57:31Z

src/plugins/kibana_utils/public/state_containers/create_state_container_react_helpers.ts

@@ -35,7 +35,7 @@ export const createStateContainerReactHelpers = <Container extends StateContaine
    return value;
  };

-  const useTransitions = () => useContainer().transitions;
+  const useTransitions: () => Container['transitions'] = () => useContainer().transitions;


This allowed to remove this type casting: https://github.com/elastic/kibana/pull/54436/files#diff-0474422dfd150c2b85e0efb48c007a20L196

src/plugins/kibana_utils/public/state_containers/types.ts

Dosant · 2020-01-10T11:11:18Z

src/plugins/kibana_utils/docs/state_containers/react.md

@@ -9,7 +9,7 @@
 ```ts
 import { createStateContainer, createStateContainerReactHelpers } from 'src/plugins/kibana_utils';

-const container = createStateContainer({}, {});
+const container = createStateContainer({});


It seemed to be reasonable to also make transitions optional. Just like selectors

Dosant · 2020-01-10T11:15:20Z

src/plugins/kibana_utils/public/state_containers/create_state_container.ts

+  defaultState: State,
+  pureTransitions: PureTransitions
+): ReduxLikeStateContainer<State, PureTransitions>;
+export function createStateContainer<


I am not exactly sure, but I ended up adding overrides here because bumped into following with previous setup just with optional generics:

const container = createStateContainer({test: 'test}, {action: (s) => () => s}); container.get().test // ts works container.transitions.action() // ts works

but
explicitly specify only State shape in generic:

type S = {test: string}; const container = createStateContainer<S>({test: 'test}, {action: (s) => () => s}); container.get().test // ts works container.transitions.action() // doesn't work.. ts now think, that transitions are {}.

So adding those overrides instead of using optional generic params just made it explicit, that when generic params are specified explicitly - the number of specified params have to match number of arguments.

elasticmachine · 2020-01-10T11:19:16Z

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

Dosant · 2020-01-10T11:20:33Z

src/plugins/kibana_utils/public/state_containers/create_state_container.ts

@@ -56,9 +74,13 @@ export const createStateContainer = <
    state$,
    getState: () => data$.getValue(),
    set: (state: State) => {
-      data$.next(freeze(state));
+      container.dispatch({ type: $$setActionType, args: [state] });


this makes set() to go through dispatch and not be skipped by added middlewares

streamich

Code LGTM. Just see one comment about BaseState.

src/plugins/kibana_utils/public/state_containers/types.ts

Dosant · 2020-01-10T12:00:39Z

I would not use Record<string, any> as that means state is basically any object, because key is any string and values of those keys are any. Maybe object can work here.

@streamich, Please see: #54436 (comment)
I would really appreciate another pair of eyes looking at it.

import { encode } from 'rison-node'
export function encodeFn<S extends object>(s: S) {
  encode(s); // error
}

import { encode } from 'rison-node'
export function encodeFn<S extends Record<string: any>>(s: S) {
  encode(s); // works
}

The error is:

TS2345: Argument of type 'S' is not assignable to parameter of type 'RisonValue'.   
Type 'object' is not assignable to type 'RisonValue'.     
Type 'object' is not assignable to type 'RisonArray'.       
Type 'S' is not assignable to type 'RisonArray'.         
Type '{}' is missing the following properties from type 'RisonArray': length, pop, push, concat, and 28 more.

streamich · 2020-01-10T12:12:49Z

Record<string, any> "works" because Record<string, any> is almost the same as any — Record<string ... means that it is an object with all possible keys of type string (basically meaning any key is allowed) and Record<..., any> means that those keys can contain any value. And when you say ... extends Record<string, any> it basically forces user to make the type of their state to be an object with all possible string keys being valid and to each of those keys it is valid to assign anything. It is very similar to ... extends any.

In your RISON example you probably need to cast the object, maybe something like this

encode(s as RisonValue);

Dosant · 2020-01-10T12:59:24Z

@streamich, used object.

In your RISON example you probably need to cast the object, maybe something like this

encode(s as RisonValue);

Looks like it :(

…agement/state-containers-improvements # Conflicts: # src/plugins/kibana_utils/public/state_containers/create_state_container.test.ts # src/plugins/kibana_utils/public/state_containers/create_state_container.ts # src/plugins/kibana_utils/public/state_containers/create_state_container_react_helpers.test.tsx # src/plugins/kibana_utils/public/state_containers/create_state_container_react_helpers.ts # src/plugins/kibana_utils/public/state_containers/types.ts

Dosant · 2020-01-13T05:47:35Z

@elasticmachine merge upstream

…ovements

kibanamachine · 2020-01-13T07:03:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8ebd6f8

History

💚 Build #19290 succeeded eed6b51
💚 Build #19260 succeeded e61e98d
💚 Build #19242 succeeded b41463a
💔 Build #19232 failed ba4a382
💔 Build #19229 failed 3425122

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

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

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

…age-offset-floating-tooltip * 'master' of github.com:elastic/kibana: [Maps] refactor isPointsOnly, isLinesOnly, and isPolygonsOnly to make synchronous (elastic#54067) Fix icon path in tutorial introduction (elastic#49684) [State Management] State containers improvements (elastic#54436) Fix floating tools rendering logic (elastic#54505) Handle another double quote special case (elastic#54474) [Home][Tutorial] Add data UI for IBM MQ Filebeat module (elastic#54238) fix(package): upgrade transitive dependency elliptic to v6.5.2 (elastic#54476) [Graph] Fix various a11y issues (elastic#54097) # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/legacy_core_editor.ts

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 added 2 commits January 10, 2020 13:45

State containers improvements

ff79197

Merge branch 'master' of github.com:elastic/kibana into dev/state-man…

a495beb

…agement/state-containers-improvements

Dosant commented Jan 10, 2020

View reviewed changes

Dosant added 2 commits January 10, 2020 14:00

add @types for deep-freeze-strict

3425122

fix example

ba4a382

Dosant commented Jan 10, 2020

View reviewed changes

src/plugins/kibana_utils/public/state_containers/types.ts Outdated Show resolved Hide resolved

Dosant commented Jan 10, 2020

View reviewed changes

Dosant added v7.7.0 Feature:StateManagement Team:AppArch labels Jan 10, 2020

Dosant added the release_note:skip Skip the PR/issue when compiling release notes label Jan 10, 2020

Dosant commented Jan 10, 2020

View reviewed changes

update renovate.json

b41463a

streamich approved these changes Jan 10, 2020

View reviewed changes

src/plugins/kibana_utils/public/state_containers/types.ts Outdated Show resolved Hide resolved

use object as BaseType

e61e98d

Dosant added 2 commits January 10, 2020 17:39

fix merge

eed6b51

Dosant added the review label Jan 13, 2020

Dosant marked this pull request as ready for review January 13, 2020 05:47

Dosant requested a review from a team as a code owner January 13, 2020 05:47

Merge branch 'master' into dev/state-management/state-containers-impr…

8ebd6f8

…ovements

Dosant added v7.6.0 and removed v7.7.0 labels Jan 13, 2020

Dosant merged commit a899df3 into elastic:master Jan 13, 2020

Dosant mentioned this pull request Jan 13, 2020

[7.x] [State Management] State containers improvements (#54436) #54562

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[State Management] State containers improvements #54436

[State Management] State containers improvements #54436

Dosant commented Jan 10, 2020 •

edited

Loading

Dosant Jan 10, 2020

Dosant Jan 10, 2020

Dosant Jan 10, 2020

elasticmachine commented Jan 10, 2020

Dosant Jan 10, 2020

streamich left a comment

Dosant commented Jan 10, 2020

streamich commented Jan 10, 2020 •

edited

Loading

Dosant commented Jan 10, 2020 •

edited

Loading

Dosant commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

[State Management] State containers improvements #54436

[State Management] State containers improvements #54436

Conversation

Dosant commented Jan 10, 2020 • edited Loading

Summary

Checklist

For maintainers

Dosant Jan 10, 2020

Choose a reason for hiding this comment

Dosant Jan 10, 2020

Choose a reason for hiding this comment

Dosant Jan 10, 2020

Choose a reason for hiding this comment

elasticmachine commented Jan 10, 2020

Dosant Jan 10, 2020

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

Dosant commented Jan 10, 2020

streamich commented Jan 10, 2020 • edited Loading

Dosant commented Jan 10, 2020 • edited Loading

Dosant commented Jan 13, 2020

kibanamachine commented Jan 13, 2020

💚 Build Succeeded

History

Dosant commented Jan 10, 2020 •

edited

Loading

streamich commented Jan 10, 2020 •

edited

Loading

Dosant commented Jan 10, 2020 •

edited

Loading