[Dashboard] [Controls] Refactor controls services #138668

Heenawter · 2022-08-11T19:35:59Z

Summary

This PR refactors the src/plugins/controls/public/services folder with the goal of aligning with the new code standards that the Presentation team is working on. It has absolutely no user-facing changes.

Checklist

N/A

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-08-12T20:24:27Z

Pinging @elastic/kibana-presentation (Team:Presentation)

Fix

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

cqliu1

This is just an initial code review. I'm loving how clean and uniform the controls code is looking! I'm gonna check out this branch and test locally before I approve.

Here are a few nits, but none of them are blockers.

src/plugins/controls/public/control_group/editor/edit_control.tsx

src/plugins/controls/public/control_group/editor/create_control.tsx

src/plugins/controls/public/control_group/editor/edit_control_group.tsx

cqliu1 · 2022-08-25T21:59:45Z

src/plugins/controls/public/options_list/types.ts

@@ -29,3 +27,5 @@ export type OptionsListReduxState = ReduxEmbeddableState<
  ControlOutput,
  OptionsListComponentState
 >;
+
+export * from '../../common/options_list/types';


Why was this line moved to the bottom of the file? Just curious

I was making it match the range_slider types file, which also has it at the bottom. But I could move both to the top instead? Don't really have a strong preference - what are your thoughts?

Hmmmmm I guess you can leave it at the bottom. It's an export so it's not like we need to group all of them at the top. We have a clear pattern for imports, but I don't think we've discussed exports. I don't know that we need to organize exports in any particular way besides just having them below imports? What do you think @ThomThomson

Where are these exports being used? Can we not just import from common in those places rather than reexporting here? I think it's usually best not to re-export if it can be avoided, because it can make things more difficult to follow.

Very fair point - the common export actually seems to be used more frequently than the re-exported public types (at least for options list - haven't checked range slider yet), so I think it makes the most sense to just avoid re-exporting stuff even if it results in shorter paths 👍 I'll make the change to this PR since it should be only a few changes

src/plugins/controls/public/services/data/types.ts

cqliu1 · 2022-08-25T22:07:47Z

src/plugins/controls/public/services/options_list/types.ts

@@ -6,7 +6,7 @@
 * Side Public License, v 1.
 */

-import { OptionsListRequest, OptionsListResponse } from '../options_list/types';
+import { OptionsListRequest, OptionsListResponse } from '../../../common/options_list/types';


Did we come to a consensus about exporting all types from the root types file in common, public, and server? I think it'd look nicer if we imported from ../../../common/types because I prefer shorter import paths, but this isn't a dealbreaker.

Hmmm... I honestly don't remember if we decided that, haha! :) I looked in to it and it seems like this would require a lottt of import changes if we wanted to be consistent with it - for example, src/plugins/controls/public/control_group/types.ts is exporting stuff from ../../common/control_group/types and so I would have to go through and change all of the imports of .../public/control_group/types.ts to use the common import instead.

Totally cool doing that, but I don't think it makes sense to make all those changes as part of this PR - maybe I could make a follow up PR that cleans up all exports/imports to make them consistent if @ThomThomson agrees that shorter import paths is better? I honestly don't have a strong preference so I'm good with either 😆

I think that within a plugin having longer import paths is fine. If we're in a file related to options_list in public and we have to import from ../../../common/options_list/types I think that works great because it ties the disparate parts of the plugin together nicely.

…enawter/kibana into refactor-controls-services_2022-08-10

cqliu1

LGTM 👍 Pulled down the changes and didn't encounter any new bugs

kibana-ci · 2022-08-29T19:48:25Z

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #47 / discover/classic "after all" hook: afterTestSuite.trigger in "discover/classic"
[job] [logs] FTR Configs #47 / discover/classic discover doc table "after all" hook: afterTestSuite.trigger for "should refresh the table content when changing time window"
[job] [logs] FTR Configs #47 / discover/classic discover doc table legacy "after all" hook: afterTestSuite.trigger for "should make the document table scrollable"
[job] [logs] FTR Configs #47 / discover/classic discover doc table legacy add and remove columns "after all" hook: afterTestSuite.trigger for "should remove columns from the table"
[job] [logs] FTR Configs #47 / discover/classic discover doc table legacy add and remove columns "after each" hook for "should add more columns to the table"
[job] [logs] FTR Configs #47 / discover/classic discover doc table legacy add and remove columns should add more columns to the table

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`controls`	195	183	-12

Async chunks

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

id	before	after	diff
`controls`	441.9KB	441.8KB	-125.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`controls`	27.0KB	25.0KB	-2.0KB

Unknown metric groups

async chunk count

id	before	after	diff
`controls`	7	6	-1

ESLint disabled line counts

id	before	after	diff
`controls`	2	1	-1

Total ESLint disabled count

id	before	after	diff
`controls`	2	1	-1

History

💔 Build #68102 failed e987d9cf936fdb15082d2f30088d367d467fbde1
💔 Build #68098 failed 09eecd40a631a123118c22de0d88ae91c16172da
💚 Build #68035 succeeded 4fcba9b
💚 Build #67579 succeeded d7486ea
💔 Build #67562 failed 119f56b
💚 Build #66903 succeeded 702144b

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

cc @Heenawter

* Restructure services folder and change all imports * Fix export of controls service * Fix import of plugin services to be async * Clean up data service to remove duplicate code Fix * Fix imports * Get rid of `useHooks` in components [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Address feedback * Remove re-exported types

Heenawter self-assigned this Aug 11, 2022

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch 2 times, most recently from e37dfc0 to ec90a48 Compare August 11, 2022 21:07

Heenawter marked this pull request as ready for review August 12, 2022 20:24

Heenawter requested a review from a team as a code owner August 12, 2022 20:24

Heenawter marked this pull request as draft August 12, 2022 20:29

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch 4 times, most recently from a01f43f to 5b64202 Compare August 15, 2022 19:01

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch 3 times, most recently from 94ebb0d to 4581e79 Compare August 22, 2022 20:47

Heenawter added 4 commits August 23, 2022 14:02

Restructure services folder and change all imports

ad99057

Fix export of controls service

d4c890e

Fix import of plugin services to be async

e5dd8a6

Clean up data service to remove duplicate code

a433791

Fix

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch from 4581e79 to b66b09c Compare August 23, 2022 20:03

Heenawter added 2 commits August 23, 2022 14:21

Fix imports

f4d9f85

Get rid of useHooks in components

9c26a01

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch from 935cc40 to 9c26a01 Compare August 23, 2022 20:21

Heenawter marked this pull request as ready for review August 23, 2022 21:24

Heenawter added 2 commits August 23, 2022 15:25

Merge branch 'main' into refactor-controls-services_2022-08-10

702144b

Merge branch 'main' into refactor-controls-services_2022-08-10

119f56b

cqliu1 reviewed Aug 25, 2022

View reviewed changes

Heenawter and others added 3 commits August 25, 2022 16:43

Address feedback

d696fe5

Merge branch 'refactor-controls-services_2022-08-10' of github.com:he…

a838ce6

…enawter/kibana into refactor-controls-services_2022-08-10

Merge branch 'main' into refactor-controls-services_2022-08-10

d7486ea

Heenawter requested a review from cqliu1 August 26, 2022 20:37

Merge branch 'main' into refactor-controls-services_2022-08-10

4fcba9b

cqliu1 approved these changes Aug 29, 2022

View reviewed changes

Heenawter enabled auto-merge (squash) August 29, 2022 18:01

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch from 09eecd4 to e987d9c Compare August 29, 2022 18:14

Remove re-exported types

114d649

Heenawter force-pushed the refactor-controls-services_2022-08-10 branch from e987d9c to 114d649 Compare August 29, 2022 18:36

Heenawter merged commit 68905bd into elastic:main Aug 29, 2022

Heenawter mentioned this pull request Feb 21, 2023

Dashboard control data does not update when editing an existing control field #141280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] [Controls] Refactor controls services #138668

[Dashboard] [Controls] Refactor controls services #138668

Heenawter commented Aug 11, 2022 •

edited

Loading

elasticmachine commented Aug 12, 2022

cqliu1 left a comment

cqliu1 Aug 25, 2022 •

edited

Loading

Heenawter Aug 25, 2022

cqliu1 Aug 29, 2022 •

edited

Loading

ThomThomson Aug 29, 2022

Heenawter Aug 29, 2022

cqliu1 Aug 25, 2022

Heenawter Aug 25, 2022 •

edited

Loading

ThomThomson Aug 29, 2022

cqliu1 left a comment

kibana-ci commented Aug 29, 2022

async chunk count

ESLint disabled line counts

Total ESLint disabled count

[Dashboard] [Controls] Refactor controls services #138668

[Dashboard] [Controls] Refactor controls services #138668

Conversation

Heenawter commented Aug 11, 2022 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 12, 2022

cqliu1 left a comment

Choose a reason for hiding this comment

cqliu1 Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Heenawter Aug 25, 2022

Choose a reason for hiding this comment

cqliu1 Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

ThomThomson Aug 29, 2022

Choose a reason for hiding this comment

Heenawter Aug 29, 2022

Choose a reason for hiding this comment

cqliu1 Aug 25, 2022

Choose a reason for hiding this comment

Heenawter Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

ThomThomson Aug 29, 2022

Choose a reason for hiding this comment

cqliu1 left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 29, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

ESLint disabled line counts

Total ESLint disabled count

History

Heenawter commented Aug 11, 2022 •

edited

Loading

cqliu1 Aug 25, 2022 •

edited

Loading

cqliu1 Aug 29, 2022 •

edited

Loading

Heenawter Aug 25, 2022 •

edited

Loading