[Lens] Move editorFrame state to redux #100858

mbondyra · 2021-05-28T10:48:35Z

Summary

Second part of #89690

Most important changes

Moving editor frame state from using useReducer to redux-toolkit store
Expose resolved datasourcesMap and visualizationMap from the editor frame service to the mounter
Moving all the loading logic from editor frame to the mounter - replace useEffects with linear code
Remove unnecessary synchronization onChange of two properties from the store: lastKnownDoc and indexPatternsForTopNav
a. Removing indexPatternsForTopNav from the global state and instead getting it in the lens_top_nav directly
b. Removing lastKnownDoc and instead calculating it from the global state in the app - could be improved further and be kept as a selector instead of keeping it in the local app state
framePublicAPI - remove methods addLayer and removeLayer and dispatch the store actions directly in chart_switcher
Replace some mutations immer forced on - nothing big, but mentioning it to avoid confusion why these changes are there
Change of behaviour: before we were able to save incorrect chart and then the error was shown in the dashboard. Now, if the expression is not built, the chart cannot be saved. (I wasn't planning on that change, it happened with the implementation changes, but I think it's an improvement)

Doubts:

Store structure is just a single slice lensSlice

To do in the future:

I kept the subtypes ‘UPDATE_ALL_STATES’, but we probably want to keep redux toolkit naming convention lens/updateAllStates
moving loading logic to the middleware
create better selectors to avoid some unnecessary rerenders 
calculate data structures which are derived from the whole state ( expression, suggestions, visualization groups, editorFrameAPI) in custom selectors using deep equality
get rid of the updaters - this is not redux way of doing this as updaters are not serialized
get rid of any not-serializable properties from state

TODO before merging:

Missing tests for mounter
missing tests for the store

elasticmachine · 2021-05-28T10:48:37Z

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

dej611 · 2021-06-18T15:51:24Z

x-pack/plugins/lens/public/app_plugin/mounter.tsx

+          })
+        );
+
+        {


nitpick: why this isolated scope?

@dej611 this code is not ready to be reviewed yet -> I marked it this way to remember about refactoring it. I'll let you know on Monday (hopefully) when it's ready!

wylieconlon

I'm not quite done reviewing this, but I did some testing today and wasn't able to break anything!

wylieconlon · 2021-06-22T22:27:09Z

x-pack/plugins/lens/public/app_plugin/app.test.tsx

@@ -177,14 +208,6 @@ describe('Lens App', () => {
    expect(services.data.query.filterManager.getFilters).not.toHaveBeenCalled();
  });

-  it('displays errors from the frame in a toast', async () => {


It looks like you haven't copied this unit test into the mounter- the onError callback was specifically called when initializeDatasources threw errors.

wylieconlon · 2021-06-22T23:14:27Z

x-pack/plugins/lens/public/types.ts

-  /**
-   * A map of all available palettes (keys being the ids).
-   */
-  availablePalettes: PaletteRegistry;


Was this availablePalettes deleted fully? I'm not seeing a replacement in this PR.

That's because they are never used. They are being loaded again in the particular visualizations that need them, for example here:
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/datatable_visualization/index.ts#L38

wylieconlon · 2021-06-22T23:19:05Z

x-pack/plugins/lens/public/state_management/lens_slice.ts

+      // This is a safeguard that prevents us from accidentally updating the
+      // wrong visualization. This occurs in some cases due to the uncoordinated
+      // way we manage state across plugins.
+      if (state.visualization.activeId !== payload.visualizationId) {


In theory both of these safeguards will be fixed by this PR!

wylieconlon · 2021-06-22T23:55:38Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.test.tsx

  });

-  it('should render render icon if there is no preview expression', () => {


Looks like this is just changing the test order?

Yes, reverting!

dej611 · 2021-06-23T13:17:03Z

Still reviewing but I think I found a bug in the current PR: when creating a percentage chart, on dropping a field on the main chart area, only the very first time, it creates a new layer where the field is added in the Break down by dimension, leaving the existing layer Break down by empty.

On master this works as expected adding the field as Break down by dimension in the current layer.

Perhaps this is a possible side effect of the removal of the addLayer from the frameAPI?

flash1293

I tested and went through the code - looks pretty good to me already. We can sync offline but I want to align about the next steps to make sure we are going into the right direction.

flash1293 · 2021-06-23T13:33:56Z

x-pack/plugins/lens/public/app_plugin/mounter.tsx

  if (
    !initialInput ||
    (attributeService.inputIsRefType(initialInput) &&
      initialInput.savedObjectId === persistedDoc?.savedObjectId)
  ) {
-    return;
+    return initializeDatasources(


It's fine for now, but I would like to move all of these things into the middleware at some point

flash1293 · 2021-06-23T13:35:56Z

x-pack/plugins/lens/public/app_plugin/mounter.tsx

+          })
+        );
+        if (initialContext) {
+          const selectedSuggestion = getVisualizeFieldSuggestions({


The whole block up to line 384 could be moved into the reduced because it's completely pure. The only thing we would need to make sure is making datasource and visualization map known to the reducer. But this will probably happen in a separate step, right?

flash1293 · 2021-06-23T13:52:17Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx

 import { uiActionsPluginMock } from '../../../../../../src/plugins/ui_actions/public/mocks';
 import { chartPluginMock } from '../../../../../../src/plugins/charts/public/mocks';
 import { expressionsPluginMock } from '../../../../../../src/plugins/expressions/public/mocks';
 import { mockDataPlugin, mountWithProvider } from '../../mocks';
+import { setState, setToggleFullscreen } from '../../state_management';
+import { FrameLayout } from './frame_layout';

 function generateSuggestion(state = {}): DatasourceSuggestion {
  return {


Can't comment there but in line 71 there's still an onError prop - seems to be a leftover

mbondyra · 2021-06-23T14:43:55Z

@dej611 thank you for your finding. It's caused somewhere by this commit: 7a8327c
I'll try to investigate more on that!

dej611

Code LGTM 👍

…_state_refactoring

flash1293 · 2021-06-29T08:32:47Z

@elasticmachine merge upstream

…_state_refactoring

…dyra/kibana into lens/editor_frame_state_refactoring

flash1293 · 2021-06-30T18:35:57Z

@elasticmachine merge upstream

kibanamachine · 2021-06-30T20:41:42Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.5MB	1.5MB	+1.5KB
`observability`	478.9KB	478.9KB	-21.0B
total			+1.5KB

Unknown metric groups

References to deprecated APIs

id	before	after	diff
`lens`	71	61	-10

History

💔 Build #135637 failed 748639d
💚 Build #135572 succeeded 4ff4cb6
💚 Build #135053 succeeded 84f219d
💔 Build #134973 failed e993af4
💔 Build #134537 failed 4146a86

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

Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: dej611 <dej611@gmail.com>

kibanamachine · 2021-07-01T09:03:55Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: dej611 <dej611@gmail.com> Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com> Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: dej611 <dej611@gmail.com>

mbondyra added Team:Visualizations v8.0.0 release_note:skip Feature:Lens v7.14.0 auto-backport labels May 28, 2021

mbondyra requested a review from a team May 28, 2021 10:48

mbondyra marked this pull request as draft May 28, 2021 10:48

mbondyra force-pushed the lens/editor_frame_state_refactoring branch 3 times, most recently from 608bdbd to 3b0304e Compare June 15, 2021 11:31

mbondyra force-pushed the lens/editor_frame_state_refactoring branch 4 times, most recently from 94dc761 to 6b22cc5 Compare June 18, 2021 13:38

dej611 reviewed Jun 18, 2021

View reviewed changes

mbondyra force-pushed the lens/editor_frame_state_refactoring branch 2 times, most recently from 225115a to 44e03d1 Compare June 18, 2021 18:32

flash1293 mentioned this pull request Jun 22, 2021

[Lens] Disable the global timepicker if user is using an index pattern without primary time field, and visualizing data without time #72502

Closed

elastic deleted a comment from kibanamachine Jun 22, 2021

mbondyra force-pushed the lens/editor_frame_state_refactoring branch 2 times, most recently from 889015b to 9fe53a5 Compare June 22, 2021 15:03

mbondyra marked this pull request as ready for review June 22, 2021 17:08

mbondyra added v7.15.0 and removed v7.14.0 labels Jun 22, 2021

wylieconlon reviewed Jun 23, 2021

View reviewed changes

mbondyra force-pushed the lens/editor_frame_state_refactoring branch from 9fe53a5 to c26a6f8 Compare June 23, 2021 09:01

mbondyra added 2 commits June 23, 2021 11:09

move editorstate to redux

37366ee

remove some effects from editor frame

4df1938

mbondyra added 3 commits June 23, 2021 11:09

wip

d86fb8c

tests for app

99a4fa6

conflicts

366d41a

mbondyra force-pushed the lens/editor_frame_state_refactoring branch from c26a6f8 to 366d41a Compare June 23, 2021 09:16

functional test fixed

cd75cab

mbondyra force-pushed the lens/editor_frame_state_refactoring branch from c84c5dc to cd75cab Compare June 23, 2021 12:05

mbondyra added 2 commits June 23, 2021 14:43

start with the state manager tests

3659b51

added more state_management tests

4be257c

mbondyra force-pushed the lens/editor_frame_state_refactoring branch from bb8fcfa to 4be257c Compare June 23, 2021 13:05

flash1293 reviewed Jun 23, 2021

View reviewed changes

dej611 approved these changes Jun 23, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into lens/editor_frame…

4146a86

…_state_refactoring

kibanamachine and others added 5 commits June 29, 2021 04:32

Merge branch 'master' into lens/editor_frame_state_refactoring

e993af4

fix functional test

84f219d

🐛 Fix layer update on switch problem

4ff4cb6

Merge remote-tracking branch 'upstream/master' into lens/editor_frame…

8e25f8c

…_state_refactoring

Merge branch 'lens/editor_frame_state_refactoring' of github.com:mbon…

748639d

…dyra/kibana into lens/editor_frame_state_refactoring

Merge branch 'master' into lens/editor_frame_state_refactoring

ad9d22e

dej611 merged commit 6e3df60 into elastic:master Jul 1, 2021

kibanamachine mentioned this pull request Jul 1, 2021

[7.x] [Lens] Move editorFrame state to redux (#100858) #104057

Merged

mbondyra mentioned this pull request Jul 22, 2021

[Lens] Restructure Lens state management #89690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Move editorFrame state to redux #100858

[Lens] Move editorFrame state to redux #100858

mbondyra commented May 28, 2021 •

edited

Loading

elasticmachine commented May 28, 2021

dej611 Jun 18, 2021

mbondyra Jun 18, 2021

wylieconlon left a comment

wylieconlon Jun 22, 2021

wylieconlon Jun 22, 2021

mbondyra Jun 23, 2021

wylieconlon Jun 22, 2021

wylieconlon Jun 22, 2021

mbondyra Jun 23, 2021

dej611 commented Jun 23, 2021

flash1293 left a comment

flash1293 Jun 23, 2021

flash1293 Jun 23, 2021

flash1293 Jun 23, 2021

mbondyra commented Jun 23, 2021 •

edited

Loading

dej611 left a comment

flash1293 commented Jun 29, 2021

flash1293 commented Jun 30, 2021

kibanamachine commented Jun 30, 2021

References to deprecated APIs

kibanamachine commented Jul 1, 2021

+                        })
+                      );
+                      {

		});

		it('should render render icon if there is no preview expression', () => {

[Lens] Move editorFrame state to redux #100858

[Lens] Move editorFrame state to redux #100858

Conversation

mbondyra commented May 28, 2021 • edited Loading

Summary

Most important changes

Doubts:

To do in the future:

elasticmachine commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Jun 23, 2021

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbondyra commented Jun 23, 2021 • edited Loading

dej611 left a comment

Choose a reason for hiding this comment

flash1293 commented Jun 29, 2021

flash1293 commented Jun 30, 2021

kibanamachine commented Jun 30, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

References to deprecated APIs

History

kibanamachine commented Jul 1, 2021

💚 Backport successful

mbondyra commented May 28, 2021 •

edited

Loading

mbondyra commented Jun 23, 2021 •

edited

Loading