[State Management] State syncing utilities - remove AppState from Dashboard app #54105

Dosant · 2020-01-07T11:35:15Z

Summary

~~Depends on #53582~~
Part of #44151
Removes AppState from dashboard app and replaces it with state containers and state syncing utilities.

There is also follow up in progress which also removes global state:
#55158

Disclamer

I didn't try to refactor the state management in Dashboard in scope of this (as new state_containers give us more flexibility).
Just tried to replace AppState -> State Containers with minimal changes

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

improve improve improve improve

elasticmachine · 2020-01-15T13:40:52Z

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

…agment/state-sync-dashboard

flash1293 · 2020-01-16T15:18:23Z

Nothing major, but probably worth looking into: If full screen mode is toggled by changing the state in the rison encoded URL parameter, data gets fetched again. I looked into this a bit and it seems like the query object gets changed triggering an angular watcher. In the current setup this doesn't happen for some reason. This could probably be handled by comparing the current state and the deserialized state from the URL and not changing the object reference if the two are deep equal.

flash1293

Some nits, but dashboard integration LGTM - works great, thanks! There is one difference I noticed but it's not a blocker for merging this. I noticed that requests are triggered in a lot of places when they are not necessary (e.g. when changing "showMargins" setting), but this is also happening on master. I will create a separate issue for that.

src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app.tsx

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx

…agment/state-sync-dashboard # Conflicts: # src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts

Dosant · 2020-01-17T17:06:19Z

Nothing major, but probably worth looking into: If full screen mode is toggled by changing the state in the rison encoded URL parameter, data gets fetched again. I looked into this a bit and it seems like the query object gets changed triggering an angular watcher. In the current setup this doesn't happen for some reason. This could probably be handled by comparing the current state and the deserialized state from the URL and not changing the object reference if the two are deep equal.

@flash1293, Good catch! this particular case is fixed by utilising "applyDiff" utility from old state_management in new state syncing utils.

There is one difference I noticed but it's not a blocker for merging this. I noticed that requests are triggered in a lot of places when they are not necessary (e.g. when changing "showMargins" setting), but this is also happening on master. I will create a separate issue for that.

Now it seems like, that it happens only when filters are applied, likely is going to be fixed together with:
#54887

lizozom

Tested and reviewed code LGTM
However, for the protocol, why did this change require adding getPendingUrl and supporting returning undefined from update in kbn_url_storage

lizozom · 2020-01-19T08:42:46Z

src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

@@ -80,7 +84,7 @@ export class FilterStateManager {

  private saveState() {
    const appState = this.getAppState();
-    if (appState) appState.save();
+    if (appState && appState.save) appState.save();


Why would we get an appState without a save method?

This filter_state_management is still used in Dashboard (until next pr, where we will remove global state also)
So, as we removed AppState from Dashboard, I change the interface of filter_state_manager to :
https://github.com/elastic/kibana/pull/54105/files/10fe7772a3297f9e5b03591dd9d47a5b8ca9be02#diff-e99af5c3c776f37f6e37ab25d094e3cfR25

And save function is for legacy AppState case, but new state sync utils don't have explicit save() method.

src/legacy/core_plugins/kibana/public/dashboard/np_ready/types.ts

…agment/state-sync-dashboard

Dosant · 2020-01-20T10:18:50Z

@lizozom,

However, for the protocol, why did this change require adding getPendingUrl and supporting returning undefined from update in kbn_url_storage

I don't remember exactly the edge case I was fighting, but in a nutshell it was something like this:

const state1 = {...}; // initial state
const state2 = {...}

setState(state2) // transition to state2, the location change was scheduled 
setState(state1) // synchronously transition back to state 1, but location change wasn't scheduled, because comparison inside stateSync util took storageState from current location.

// so we ended up in state1, but url with state2 :(

Dosant · 2020-01-20T14:06:48Z

retest

…agment/state-sync-dashboard

Dosant · 2020-01-21T09:03:46Z

@elasticmachine merge upstream

…agment/state-sync-dashboard # Conflicts: # src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

Dosant · 2020-01-21T10:28:15Z

retest

kibanamachine · 2020-01-21T11:42:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 446c744

History

💔 Build #21499 failed 446c744
💚 Build #21430 succeeded 3f7a2b8
💚 Build #21393 succeeded 23845a5
💚 Build #21366 succeeded 23845a5
💔 Build #21341 failed 1257fb1

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

Removes AppState from dashboard app and replaces it with state containers and state syncing utilities.

* master: [State Management] remove AppState from Dashboard app (elastic#54105) Expose fatalErrors API from the Start contract (elastic#55300) [BUG] Data fetching twice on discover timefilter change (elastic#55279) [Mappings editor] Add missing max_shingle_size parameter to search_as_you_type (elastic#55161) [Logs UI] Fix z-index of logs page toolbar (elastic#54469) removes CTA from Task Manager info message (elastic#55334)

Removes AppState from dashboard app and replaces it with state containers and state syncing utilities. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Dosant mentioned this pull request Jan 7, 2020

[State Management] State syncing utilities #53582

Merged

2 tasks

streamich mentioned this pull request Jan 13, 2020

State Management 👉 New Platform #44151

Closed

30 tasks

Dosant force-pushed the dev/state-managment/state-sync-dashboard branch 2 times, most recently from c7a861e to 415095b Compare January 14, 2020 15:37

Remove AppState from np/dashboard

5073715

improve improve improve improve

Dosant force-pushed the dev/state-managment/state-sync-dashboard branch from 415095b to 5073715 Compare January 14, 2020 15:45

Dosant added 2 commits January 15, 2020 11:53

fix hard reseting time filters

5b042db

make sure url ('_a') matches initial state

fb47fc8

Dosant added v7.7.0 Feature:New Platform Feature:StateManagement Feature:Dashboard Dashboard related features review labels Jan 15, 2020

Dosant added this to In progress in kibana-app-arch via automation Jan 15, 2020

Dosant added the Team:AppArch label Jan 15, 2020

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

Dosant changed the title ~~[WIP][State Management] State syncing utilities - remove AppState from Dashboard app~~ [State Management] State syncing utilities - remove AppState from Dashboard app Jan 15, 2020

Dosant marked this pull request as ready for review January 15, 2020 13:41

Dosant requested a review from a team January 15, 2020 13:41

Dosant requested a review from a team as a code owner January 15, 2020 13:41

Dosant requested review from stacey-gammon and flash1293 January 15, 2020 13:42

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

9fced16

…agment/state-sync-dashboard

flash1293 approved these changes Jan 16, 2020

View reviewed changes

Dosant mentioned this pull request Jan 17, 2020

[State Management] - Remove GlobalState from dashboard #55158

Merged

7 tasks

Dosant added 2 commits January 17, 2020 12:59

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

262912e

…agment/state-sync-dashboard # Conflicts: # src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts

improvements according to review

d15cd9b

clean up AppState imports

10fe777

lizozom approved these changes Jan 19, 2020

View reviewed changes

kibana-app-arch automation moved this from In progress to Review in progress Jan 19, 2020

Dosant added 2 commits January 20, 2020 10:50

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

873aa19

…agment/state-sync-dashboard

fix used before declare

1257fb1

Dosant added 2 commits January 20, 2020 12:43

fixing test

6e71ffb

improve stability of dashboard clone test

23845a5

Dosant added the v8.0.0 label Jan 20, 2020

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

3f7a2b8

…agment/state-sync-dashboard

elasticmachine and others added 2 commits January 21, 2020 04:03

Merge branch 'master' into dev/state-managment/state-sync-dashboard

45bd639

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

446c744

…agment/state-sync-dashboard # Conflicts: # src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts

Dosant merged commit 27c8a4b into elastic:master Jan 21, 2020

kibana-app-arch automation moved this from Review in progress to Done in 7.7 Jan 21, 2020

Dosant mentioned this pull request Jan 21, 2020

[7.x] [State Management] remove AppState from Dashboard app (#54105) #55375

Merged

Dosant added a commit to Dosant/kibana that referenced this pull request Jan 21, 2020

[State Management] remove AppState from Dashboard app (elastic#54105)

84f4974

Removes AppState from dashboard app and replaces it with state containers and state syncing utilities.

Dosant mentioned this pull request Jan 21, 2020

[Discuss] Syncing filters & time with URL using new state management utils #55339

Closed

Dosant mentioned this pull request Jan 31, 2020

[State Management] State syncing utils docs #56479

Merged

7 tasks

flash1293 mentioned this pull request Jan 31, 2020

Remove AppState in Visualize #56492

Closed

This was referenced Feb 7, 2020

Dashboard: old panel still present after it was replaced with the new one #55992

Closed

Unskip replace flyout test #57093

Merged

alexh97 moved this from Done in current release to Done in previous release in kibana-app-arch Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[State Management] State syncing utilities - remove AppState from Dashboard app #54105

[State Management] State syncing utilities - remove AppState from Dashboard app #54105

Dosant commented Jan 7, 2020 •

edited

Loading

elasticmachine commented Jan 15, 2020

flash1293 commented Jan 16, 2020 •

edited

Loading

flash1293 left a comment

Dosant commented Jan 17, 2020 •

edited

Loading

lizozom left a comment

lizozom Jan 19, 2020

Dosant Jan 20, 2020

Dosant commented Jan 20, 2020 •

edited

Loading

Dosant commented Jan 20, 2020

Dosant commented Jan 21, 2020

Dosant commented Jan 21, 2020

kibanamachine commented Jan 21, 2020

[State Management] State syncing utilities - remove AppState from Dashboard app #54105

[State Management] State syncing utilities - remove AppState from Dashboard app #54105

Conversation

Dosant commented Jan 7, 2020 • edited Loading

Summary

Disclamer

Checklist

For maintainers

elasticmachine commented Jan 15, 2020

flash1293 commented Jan 16, 2020 • edited Loading

flash1293 left a comment

Choose a reason for hiding this comment

Dosant commented Jan 17, 2020 • edited Loading

lizozom left a comment

Choose a reason for hiding this comment

lizozom Jan 19, 2020

Choose a reason for hiding this comment

Dosant Jan 20, 2020

Choose a reason for hiding this comment

Dosant commented Jan 20, 2020 • edited Loading

Dosant commented Jan 20, 2020

Dosant commented Jan 21, 2020

Dosant commented Jan 21, 2020

kibanamachine commented Jan 21, 2020

💚 Build Succeeded

History

Dosant commented Jan 7, 2020 •

edited

Loading

flash1293 commented Jan 16, 2020 •

edited

Loading

Dosant commented Jan 17, 2020 •

edited

Loading

Dosant commented Jan 20, 2020 •

edited

Loading