[State Management] - Remove GlobalState from dashboard by Dosant · Pull Request #55158 · elastic/kibana

Dosant · 2020-01-17T10:45:36Z

Summary

Part of #44151
Removes GlobalState from dashboard app
~~depends on #54105~~

Open for discussion: #55339 where to put and how to abstract syncing filters & time - https://github.com/elastic/kibana/pull/55158/files#diff-5e89f9f26cbd7f233495a2e97789a306R51
And how to make syncing of not "pinned" filters not an application concern - https://github.com/elastic/kibana/pull/55158/files#diff-47ad20cc6caa58a3e9c204106013514fR151

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

elasticmachine · 2020-01-21T12:25:59Z

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

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

flash1293

It seems like the browser history isn't written correctly in all cases when filters get pinned.

To reproduce:

Create filter pill
Pin the filter
Change something in the pilled filter (e.g. invert)
Press back button (action gets undone correctly)
Press back button again (I would expect the filter to become unpinned) -> nothing happens
Press back button again -> filter disappears completely and I can't go forward anymore (history is lost)

src/plugins/data/public/query/state_sync/sync_filters.ts

src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/sync_app_filters.ts

lizozom

Overall looks good.
Added another minor comment.

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

src/plugins/data/public/query/filter_manager/filter_manager.ts

Dosant · 2020-01-23T12:07:21Z

It seems like the browser history isn't written correctly in all cases when filters get pinned.

@flash1293, thanks for finding this bug! Indeed edge case when filter change mutates both app filters and global filters. (Pin -> add to global & remove from app).

This should be fixed now

src/plugins/data/public/query/filter_manager/filter_manager.ts

src/plugins/data/public/query/state_sync/index.ts

flash1293

Tested and everything works fine for me, thanks for this! I left one comment suggesting light restructuring that would make the code safer and easier to get IMHO, but I don't consider it a blocker because I will continue working on that area of the code soon.

flash1293 · 2020-01-23T15:27:52Z

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

+const angularGlobalStateHacks: AngularGlobalStateHacks = {};

 export const renderApp = (element: HTMLElement, appBasePath: string, deps: RenderDeps) => {
+  const history = createHashHistory();


nit: We could put that part into a function initGlobalStateHandling that is called in the dashboard controller in src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx and in the listing controller in src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js - initGlobalStateHandling would return a function that stops the syncing and can be registered within the controller using $scope.$on('$destroy', () => {. Then we don't need a reference on the application level and don't need to overwrite typescripts type checks

thanks, looks better 👍
Hope I got it right.
3a51104

…agement/global-state-dashboard # Conflicts: # src/plugins/data/public/query/filter_manager/filter_manager.test.ts

…agement/global-state-dashboard

lizozom · 2020-01-26T10:26:21Z

src/plugins/dashboard_embeddable_container/public/actions/replace_panel_flyout.tsx

    const newObj = await this.props.container.addSavedObjectEmbeddable(type, id);

-    const finalPanels = this.props.container.getInput().panels;
+    const finalPanels = _.cloneDeep(this.props.container.getInput().panels);


What changed here, that made you clone the panels?

State containers deeply freeze the object in dev mode, which allows to find places where we unintentionally mutate objects passed into services. The freezing pointed me to this replace_panel_flyout:

Tweaked the code to not directly mutate original panels

Fixed the call container.updateInput(finalPanels) -> container.updateInput({panels: finalPanels). Typescript didn't complain because updates

We discussed it with @streamich.
And in the end I decided that it would be easier to fix this piece of code in replace_panel_flyout then making sure that all places where data is passed from state container into embeddable container is cloneDeeped

lizozom · 2020-01-26T10:28:16Z

src/plugins/data/public/query/filter_manager/filter_manager.ts

+  public static setFiltersStore(
+    filters: esFilters.Filter[],
+    store: esFilters.FilterStateStore,
+    shouldOverrideStore = false


Why did you have to add this?

These make sense to me in case of setGlobalFilters and setAppFilters.
This what I was thinking:

In both of those methods we need to handle the case, when filters with incorrect "$store" value are passed in

We have couple options then:

Just ignore such and do nothing

Throw an error

Override - I went with this one.

lizozom · 2020-01-26T12:43:25Z

@elasticmachine merge upstream

kibanamachine · 2020-01-26T14:02:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3106587

History

💚 Build #22399 succeeded 1ebb3a1
💚 Build #22342 succeeded 7203901
💚 Build #22328 succeeded 15d1525
💚 Build #22231 succeeded e561d06
💚 Build #22099 succeeded 8d0d0df

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

lizozom

Tested and seems to be working.

I think the external APIs for syncing can be improved, but for now, this is a good step forward.
However, I wouldn't implement this solution in other plugins before improving the interfaces in a follow up PR.
I'll create an issue to discuss this.

Removes GlobalState from dashboard app

Dosant mentioned this pull request Jan 17, 2020

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

Merged

7 tasks

Dosant force-pushed the dev/state-management/global-state-dashboard branch from d0bbbf5 to 51043ea Compare January 21, 2020 12:09

Dosant mentioned this pull request Jan 21, 2020

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

Closed

Dosant added review Team:AppArch labels Jan 21, 2020

Dosant added v8.0.0 v7.7.0 Feature:Dashboard Dashboard related features release_note:skip Skip the PR/issue when compiling release notes labels Jan 21, 2020

Dosant marked this pull request as ready for review January 21, 2020 12:27

Dosant requested review from a team, flash1293, lizozom and ppisljar January 21, 2020 12:27

Dosant commented Jan 21, 2020

View reviewed changes

src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx Outdated Show resolved Hide resolved

Dosant changed the title ~~[wip][State Management] - Remove GlobalState from dashboard~~ [State Management] - Remove GlobalState from dashboard Jan 21, 2020

Dosant force-pushed the dev/state-management/global-state-dashboard branch from 51043ea to c567e32 Compare January 21, 2020 14:40

Dosant added 3 commits January 22, 2020 10:59

Remove GlobalState from Dashboard App

3ac4119

move filters syncing utility to data plugin

b343638

improve

79850bf

Dosant force-pushed the dev/state-management/global-state-dashboard branch from b60cad0 to 79850bf Compare January 22, 2020 10:28

flash1293 reviewed Jan 22, 2020

View reviewed changes

src/plugins/data/public/query/state_sync/sync_filters.ts Outdated Show resolved Hide resolved

ppisljar reviewed Jan 22, 2020

View reviewed changes

src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/sync_app_filters.ts Outdated Show resolved Hide resolved

lizozom reviewed Jan 22, 2020

View reviewed changes

src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts Outdated Show resolved Hide resolved

Dosant added 2 commits January 22, 2020 18:36

test

7189cf6

fix bug

4ad51c3

Dosant mentioned this pull request Jan 22, 2020

[Bug][Filters] Incorrect result, when calling setFilters() with multiple duplicates between app and global filters #55591

Merged

7 tasks

add setGlobalFilters & setGlobalFilters methods

74acc33

Dosant commented Jan 23, 2020

View reviewed changes

src/plugins/data/public/query/filter_manager/filter_manager.ts Show resolved Hide resolved

Dosant commented Jan 23, 2020

View reviewed changes

src/plugins/data/public/query/filter_manager/filter_manager.ts Show resolved Hide resolved

Dosant commented Jan 23, 2020

View reviewed changes

src/plugins/data/public/query/state_sync/index.ts Show resolved Hide resolved

flash1293 approved these changes Jan 23, 2020

View reviewed changes

Dosant added 8 commits January 23, 2020 17:00

fix bug with “replace” which mutated object that shouldn’t be muted

76ceccd

improve comparisions

1fbf97f

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

e561d06

…agement/global-state-dashboard # Conflicts: # src/plugins/data/public/query/filter_manager/filter_manager.test.ts

improve according to review

3a51104

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

fdf7d2e

…agement/global-state-dashboard

fix import

15d1525

tests for sync_app_filters

7203901

add test for sync_query

1ebb3a1

lizozom reviewed Jan 26, 2020

View reviewed changes

Merge branch 'master' into dev/state-management/global-state-dashboard

3106587

lizozom approved these changes Jan 27, 2020

View reviewed changes

Dosant merged commit 987dfbc into elastic:master Jan 27, 2020

Dosant mentioned this pull request Jan 27, 2020

[7.x] [State Management] - Remove GlobalState from dashboard (#55158) #55979

Merged

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

[State Management] - Remove GlobalState from dashboard (elastic#55158)

1163201

Removes GlobalState from dashboard app

Dosant deleted the dev/state-management/global-state-dashboard branch January 27, 2020 09:52

Dosant added a commit that referenced this pull request Jan 27, 2020

[State Management] - Remove GlobalState from dashboard (#55158) (#55979)

652b6ae

Removes GlobalState from dashboard app

This was referenced Jan 29, 2020

[Bugfix][State Management] 'state:storeInSessionStorage' doesn’t take effect for dashboard without full page reload #56284

Merged

[State Management] State syncing utils docs #56479

Merged

streamich mentioned this pull request Jan 31, 2020

State Management 👉 New Platform #44151

Closed

30 tasks

wylieconlon mentioned this pull request Feb 3, 2020

[7.6.1] [Lens] Fix bugs in Lens filters (#56441) #56648

Merged

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

Conversation

Dosant commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jan 21, 2020

Uh oh!

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dosant commented Jan 23, 2020

Uh oh!

Uh oh!

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

Dosant Jan 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizozom Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Dosant Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizozom Jan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Dosant Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

lizozom commented Jan 26, 2020

Uh oh!

kibanamachine commented Jan 26, 2020

💚 Build Succeeded

History

Uh oh!

lizozom left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Dosant commented Jan 17, 2020 •

edited

Loading

Dosant Jan 24, 2020 •

edited

Loading

Dosant Jan 27, 2020 •

edited

Loading

lizozom left a comment •

edited

Loading