[Visualize] Remove legacy appState in visualize by sulemanof · Pull Request #57330 · elastic/kibana

sulemanof · 2020-02-11T15:59:33Z

Summary

Remove legacy appState in visualize and use new platform utilities for state management

Resolves #56492

remove usage of legacy AppState in visualize;
replace AppState.makeStateful method with a local makeStateful utility. This is used to create an instnace of PersistedState for the uiState object, and seems is was used only in visualize (it's also used in discover now, but will be removed), so could be safely removed with the AppState class;
use createStateContainer utility for app state management;
use createKbnUrlStateStorage utility for url state management;
use syncState to bound the appState and the url state updates;

Also resolves #55084

Manipulating with history back/forward :

Checklist

Delete any items that are not applicable to this PR.

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
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

…p_state

elasticmachine · 2020-02-12T12:08:43Z

Pinging @elastic/kibana-app (Team:KibanaApp)

sulemanof · 2020-02-12T12:10:25Z

@Bargs I would kindly ask you to test the query functionality due to this changes! 😊

…p_state

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/lib/visualize_app_state.ts

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

kertal · 2020-02-13T10:20:25Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

-    stateMonitor = stateMonitorFactory.create($state, stateDefaults);
-    stateMonitor.ignoreProps(['vis.listeners']).onChange(status => {
-      $appStatus.dirty = status.dirty || !savedVis.id;
-    });


Since you've removed the stateMonitor (which is good), I might be wrong but $appStatus.dirty is just initialized and no longer changed?

Hey, yeah!
I left one initializing reference to it intentionally!
I would highlight the thing about the $appStatus, because I didn't find a proper case how should it work!
So, if someone has a vision about how should this work, he can mention it here.
If it's redundant, I'll remove it!

I'll take a closer look here

$appStatus.isDirty is used when you want to share a visualization, but it has not been saved before, or there were changes after saving.

kibana/src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

Line 211 in 483ccc8

const hasUnsavedChanges = $appStatus.dirty;

kibana/src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

Line 222 in 483ccc8

isDirty: hasUnappliedChanges || hasUnsavedChanges,

When you then try to generate a report you get the following message:

"Please save your work before generating a report."

So $appStatus.isDirty should be set to true, when the visualization was not saved, or it was saved and the appState changed.
It should be set to false once the visualization was saved. And this persisted state is the state that you need to check agains for further changes that modify $appStatus.isDirty

@kertal thanks for the deep explanation!
Applied dirty state checker due to current code

flash1293

This looks really good! Found two small things but overall great work!

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

flash1293 · 2020-02-13T15:32:39Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

+    if (!$scope.linked) return;

-    $state.linked = false;
+    $scope.linked = false;


When testing the link/unlink saved search did not work when using browser back/forward buttons - probably because the linked state is saved outside of the state? Would it make sense to also put it in app state? Looks like it is currently part of it.

Good catch! Updated!

flash1293 · 2020-02-14T13:05:59Z

Noticed a small bug - when pinning a filter, you have to hit the back button twice to get back to the old non-pinned status - it seems like it's writing two history entries for this case. Maybe that's because it is half angular / half utils state management - also fine with fixing it when migrating global state, but we should create an issue in this case to make sure not to forget it.

…p_state # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

sulemanof · 2020-02-17T15:14:17Z

Noticed a small bug - when pinning a filter, you have to hit the back button twice to get back to the old non-pinned status - it seems like it's writing two history entries for this case. Maybe that's because it is half angular / half utils state management - also fine with fixing it when migrating global state, but we should create an issue in this case to make sure not to forget it.

Good catch @flash1293 !
I've created an issue to track this, will manage it while migrating the GlobalState.

flash1293 · 2020-02-18T13:37:02Z

@sulemanof about the linked saved search:

It seems like this doesn't work completely - it is putting the link back in place, but it's not updating the search source behind the scenes. The filters are just gone.

Example (saved search filtering by category "Womens clothing"):

after unlinking saved search (correct behavior)

after pressing back button (changed results, seems like the filters aren't put back in place):

Not sure whether that ever worked because it seems like the unlink function is triggering quite some side effects that are probably difficult to roll back, but if it's an easy fix we should definitely include it. If it's difficult to fix let's just revert to the old state and have it living outside of history, that would be less confusing. Sorry in that cause for bringing it up.

flash1293 · 2020-02-18T13:56:18Z

Besides the problem above everything else works as expected for me.

…p_state # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts

…p_state

sulemanof · 2020-02-19T08:56:09Z

Hey @flash1293
The point you have mentioned abot the linked search really worth a fix!
Since this behavior is not caused by this PR and after a short look inside I don't have an easy fix (at least it will be more than couple LOC), I've created a separate issue #57977 to track it and to create a purposeful PR, which could also cause further discussing; this also will be mentioned as a separate task for git history.
I think this will also help reviewers to concentrate on a particular case.
I'll be happy to take care of it, but not to block current PR and to minimize next merge conflicts here.
Are you OK with that? Thanks =)

flash1293 · 2020-02-19T09:04:08Z

@sulemanof Sounds good to me, thanks for creating the issue.

kibanamachine · 2020-02-19T10:41:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7c2e66b

History

💚 Build #27193 succeeded ef33f8f
💚 Build #26469 succeeded 483ccc8
💚 Build #26202 succeeded fe2da5a
💔 Build #26149 failed 7724450
💔 Build #26118 failed 31944ff

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

Dosant

Code looks good to me,
Tested some advanced cases like switching between hashed / expanded url format, navigating between vis / dashboard and using recently viewed items - didn't notice any issues.

Dosant · 2020-02-19T16:05:52Z

src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/lib/visualize_app_state.ts

+
+export function useVisualizeAppState({ useHash, stateDefaults }: Arguments) {
+  const history = createHashHistory();
+  const kbnUrlStateStorage = createKbnUrlStateStorage({


Just for info: to fix pinned/unpinned filter, it will be important that global state syncing uses the same kbnUrlStateStorage as app state syncing.

flash1293

Tested and works as expected, LGTM 👍

kertal

Code LGTM, "dirty" state is handled correctly, great work! tested locally in Chrome

* master: (136 commits) [Visualize] Remove legacy appState in visualize (elastic#57330) Use static time for tsvb rollup test (elastic#57701) [SIEM] Fix ResizeObserver polyfill (elastic#58046) [SIEM][Detection Engine] Fixes return codes where some were rule_id instead of id skip flaky suite (elastic#56816) skip flaky suite (elastic#58059) skip flaky suite (elastic#45348) migrates notification server routes to NP (elastic#57906) Moved all of the show/hide toggles outside of ordered lists. (elastic#57163) [APM] NP Migration - Moves plugin server files out of legacy (elastic#57532) [Maps][Telemetry] Migrate Maps telemetry to NP (elastic#55055) Embeddable add panel examples (elastic#57319) Fix useRequest to support query change (elastic#57723) Allow custom paths in plugin generator (elastic#57766) [SIEM][Case] Merge header components (elastic#57816) [ML] New Platform server shim: update job audit messages routes (elastic#57925) [kbn/optimizer] emit success event from reducer when all bundles cached (elastic#57945) [APM] Don’t include UI filters when fetching a specific transaction (elastic#57934) Upgrade yargs (elastic#57720) skip flaky suite (elastic#57762) (elastic#57997) (elastic#57998) ... # Conflicts: # src/plugins/advanced_settings/public/management_app/components/field/__snapshots__/field.test.tsx.snap # src/plugins/advanced_settings/public/management_app/components/field/field.tsx # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

* Remove legacy appState in visualize * Read query and linked prop from scope * Fix persisted state instance * Fix functional tests * Bound url updates with an editor * Some improvements * Fix comments

sulemanof added 7 commits February 11, 2020 18:57

Remove legacy appState in visualize

aa1200a

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

022b4bc

…p_state

Read query and linked prop from scope

4a6d82f

Fix persisted state instance

d36400a

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

31944ff

…p_state

Fix functional tests

63fc880

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

7724450

…p_state

sulemanof requested review from Bargs, flash1293 and kertal February 12, 2020 12:07

sulemanof added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.7.0 v8.0.0 labels Feb 12, 2020

sulemanof marked this pull request as ready for review February 12, 2020 12:08

sulemanof requested a review from a team February 12, 2020 12:08

sulemanof requested a review from a team as a code owner February 12, 2020 12:08

sulemanof added 3 commits February 12, 2020 15:14

Remove dev helper string

66de072

Bound url updates with an editor

69b8ddb

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

fe2da5a

…p_state

kertal reviewed Feb 13, 2020

View reviewed changes

Some improvements

483ccc8

flash1293 reviewed Feb 13, 2020

View reviewed changes

kertal requested a review from Dosant February 14, 2020 10:55

sulemanof added 2 commits February 17, 2020 13:51

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

fc8e5a4

…p_state # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js

Fix comments

ef33f8f

sulemanof mentioned this pull request Feb 18, 2020

Change the default name of a visualization #57100

Closed

sulemanof added 2 commits February 18, 2020 17:59

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

caef5bc

…p_state # Conflicts: # src/legacy/core_plugins/kibana/public/visualize/legacy_imports.ts

Merge remote-tracking branch 'upstream/master' into shim/visualize_ap…

7c2e66b

…p_state

Dosant approved these changes Feb 19, 2020

View reviewed changes

kertal self-requested a review February 20, 2020 09:27

flash1293 approved these changes Feb 20, 2020

View reviewed changes

kertal approved these changes Feb 20, 2020

View reviewed changes

sulemanof merged commit 0b64411 into elastic:master Feb 20, 2020

sulemanof deleted the shim/visualize_app_state branch February 20, 2020 10:40

sulemanof mentioned this pull request Feb 20, 2020

[7.x] [Visualize] Remove legacy appState in visualize (#57330) #58105

Merged

sulemanof added the backported label Feb 20, 2020

flash1293 mentioned this pull request May 4, 2020

Link to Vizualization with Query Broken #64989

Closed

Conversation

sulemanof commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Remove legacy appState in visualize and use new platform utilities for state management

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 12, 2020

Uh oh!

sulemanof commented Feb 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kertal Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

sulemanof Feb 13, 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!

kertal Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

kertal Feb 14, 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!

sulemanof Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flash1293 Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

sulemanof Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Feb 14, 2020

Uh oh!

sulemanof commented Feb 17, 2020

Uh oh!

flash1293 commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Feb 18, 2020

Uh oh!

sulemanof commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Feb 19, 2020

Uh oh!

kibanamachine commented Feb 19, 2020

💚 Build Succeeded

History

Uh oh!

Dosant left a comment

Choose a reason for hiding this comment

Uh oh!

Dosant Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sulemanof commented Feb 11, 2020 •

edited

Loading

sulemanof Feb 13, 2020 •

edited

Loading

kertal Feb 14, 2020 •

edited

Loading

flash1293 commented Feb 18, 2020 •

edited

Loading

sulemanof commented Feb 19, 2020 •

edited

Loading