[App Search] Refactor AppLogic to initialize data via props rather than action #92841

cee-chen · 2021-02-25T16:40:19Z

Summary

I strongly recommend following along per-commit in this PR. It's relatively small, but there's some jumps/test cleanups, and I also tried to leave very detailed notes in my commit messages.

Before

Per recent discovery from @byronhulcher and @JasonStoltz, we were running into "flash" scenarios due to the way we were initializing data in our AppLogic file. Because we were initializing with default empty objs and then calling initializeAppData on load, this was leading to the following scenario:

This was particularly problematic on routings wrapped in role capabilities, where one route would briefly and erroneously load before another.

After

With this new fix, any data in AppLogic should only be initialized once, never as falsy/undefined (unless something has gone seriously wrong with our config API endpoint, in which case the entirely plugin should not register as loaded):

Also, we can now refer to configuredLimits without having to add extra logic for potential falsy-ness all the dang time.

QA

App runs as before with no regressions, ability checks work as expected

Checklist

Unit or functional tests were updated or added to match the most common scenarios

cee-chen · 2021-03-01T16:27:13Z

@elasticmachine merge upstream

byronhulcher

Woo, glad to see us using the props pattern more!

byronhulcher · 2021-03-01T16:41:57Z

x-pack/plugins/enterprise_search/common/types/index.ts

@@ -32,6 +29,11 @@ export interface ConfiguredLimits {
  workplaceSearch: WorkplaceSearchConfiguredLimits;
 }

+export interface Access {


EnterpriseSearchAccess? I honestly am not sure how much I feel like we should be namespacing these in some way more but Access feels very generic to me

Maybe ProductAccess? I think the thing I struggle with with the EnterpriseSearch prefix is that it feels it either potentially applies to 1. the entire solution, or 2. both products (in this case not correct), or 3. the specific Enterprise Search overview plugin.

byronhulcher · 2021-03-01T16:43:35Z

x-pack/plugins/enterprise_search/public/applications/app_search/app_logic.ts

+    configuredLimits: [props.configuredLimits.appSearch, {}],
+    ilmEnabled: [props.ilmEnabled, {}],


(thinking out loud) it seems wrong that these are a part of the logic but never change.

I had this hesitation at first too, but I think @JasonStoltz managed to convince me it's not all that weird. We do this in a decent amount of our shared logic files, e.g. Kibana values/helpers, and it's easier to think about it as 'shared context' and not 'shared state' (i.e., it doesn't necessarily need to change or update). Kea at this point is basically a standard way of hooking into / sharing static or dynamic data across all our plugins, and from a dev QOL perspective makes accessing that data quick and easy (e.g., no need to use multiple Provider wrappers).

Yeah, this is a bit like you were talking about the other day Byron, about putting configuration in a React Context to pass down throughout your component tree.

This is the same as that, but instead of using a React Context directly, you're using Kea. And the nice thing about that is that you don't have to deal with Contexts AND Kea in your code, you just always use Kea.

cee-chen · 2021-03-01T21:14:35Z

@byronhulcher Oh I totally forgot to mention, I'd like to hold off merging this in until your meta engines PR lands, so I can remove your configured Limits = fallback in this PR! (#92127 (comment))

EDIT: done f38d6ad

JasonStoltz · 2021-03-02T15:02:43Z

x-pack/plugins/enterprise_search/public/applications/app_search/app_logic.ts

-  configuredLimits: Partial<ConfiguredLimits>;
-  account: Partial<Account>;
-  myRole: Partial<Role>;
+  configuredLimits: ConfiguredLimits;


Oh man, that's so much better.

JasonStoltz

So much better, thanks for doing this Constance.

- it was being duplicated in server/check_access and InitialAppData + add mock access data to DEFAULT_INITIAL_APP_DATA + update server/ tests to account for access in DEFAULT_INITIAL_APP_DATA

…alizeAppData + update tests to rerender a wrapper rather than doing {...DEFAULT_INITIAL_APP_DATA} repeatedly

- main goal of this PR is to prevent the flash of state between mount and initializeX being called - note: I recommend turning off whitespace changes in the test file

- which it should be in any case in a production environment - note: I could have changed InitialAppData to remove the ? optional notation, but decided on this route instead since InitialAppData affects more than just App Search (e.g. server, WS), and I didn't want this to have potential far-reaching side effects

…otentially undefined - which is mostly just configuredLimits it looks like

kibanamachine · 2021-03-02T19:20:07Z

💚 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
`enterpriseSearch`	2.1MB	2.1MB	-957.0B
`triggersActionsUi`	1.6MB	1.5MB	-23.9KB
total			-24.8KB

Page load bundle

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

id	before	after	diff
`triggersActionsUi`	104.0KB	104.1KB	+82.0B

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	41	42	+1

History

💔 Build #110412 failed 1fd7c4022676eb7f2c5a6f1d729d7f0ecf323aa3
💚 Build #110107 succeeded a2feedae77d7e49b1de829f7439eb3c415bd0bca
💚 Build #109973 succeeded a29eedfe9beac6090cc47328ee3615952e6940ae
💚 Build #109466 succeeded 34f28787a4b72d7fc8479d5ffaf1b7a11d4708f2

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

cc @JasonStoltz

…an action (elastic#92841) * [Misc cleanup] Move Access type to common - it was being duplicated in server/check_access and InitialAppData + add mock access data to DEFAULT_INITIAL_APP_DATA + update server/ tests to account for access in DEFAULT_INITIAL_APP_DATA * Update AppSearchConfigured to pass props to AppLogic vs calling initializeAppData + update tests to rerender a wrapper rather than doing {...DEFAULT_INITIAL_APP_DATA} repeatedly * Update AppLogic to set values from props rather than a listener - main goal of this PR is to prevent the flash of state between mount and initializeX being called - note: I recommend turning off whitespace changes in the test file * Update AppLogic typing so that app data is always expected - which it should be in any case in a production environment - note: I could have changed InitialAppData to remove the ? optional notation, but decided on this route instead since InitialAppData affects more than just App Search (e.g. server, WS), and I didn't want this to have potential far-reaching side effects * Update type scenarios where AppLogic values were previously thought potentially undefined - which is mostly just configuredLimits it looks like * [PR feedback] Type name

kibanamachine · 2021-03-02T19:28:44Z

💚 Backport successful

✅ 7.x / #93292

Successful backport PRs will be merged automatically after passing CI.

…an action (#92841) (#93292) * [Misc cleanup] Move Access type to common - it was being duplicated in server/check_access and InitialAppData + add mock access data to DEFAULT_INITIAL_APP_DATA + update server/ tests to account for access in DEFAULT_INITIAL_APP_DATA * Update AppSearchConfigured to pass props to AppLogic vs calling initializeAppData + update tests to rerender a wrapper rather than doing {...DEFAULT_INITIAL_APP_DATA} repeatedly * Update AppLogic to set values from props rather than a listener - main goal of this PR is to prevent the flash of state between mount and initializeX being called - note: I recommend turning off whitespace changes in the test file * Update AppLogic typing so that app data is always expected - which it should be in any case in a production environment - note: I could have changed InitialAppData to remove the ? optional notation, but decided on this route instead since InitialAppData affects more than just App Search (e.g. server, WS), and I didn't want this to have potential far-reaching side effects * Update type scenarios where AppLogic values were previously thought potentially undefined - which is mostly just configuredLimits it looks like * [PR feedback] Type name Co-authored-by: Constance <[email protected]>

scottybollinger · 2021-03-02T22:39:58Z

x-pack/plugins/enterprise_search/public/applications/app_search/index.tsx

    myRole: { canManageEngines, canManageMetaEngines },
-  } = useValues(AppLogic);
+  } = useValues(AppLogic(props));


TIL there is another way to do this. Definitely think what we have is the right solution but it's nice to see that we could do something else in a component, if needed. Particularly if we needed shared props in multiple child components

TIL!!! Thanks Scotty, that's incredibly helpful if we start diving into keyed logic more!

Yeah, I'm beginning the process of migrating App Search's Role Mappings and see a lot of shared code in our logic files. Thinking of either creating a shared logic file or finding a way to inherit reducers and actions from some sort of shared parent logic file.

cee-chen requested review from JasonStoltz, byronhulcher and a team February 25, 2021 16:40

cee-chen force-pushed the app-logic branch from 08bb4a8 to 34f2878 Compare February 25, 2021 16:49

cee-chen mentioned this pull request Feb 25, 2021

[App Search] Create Curation view/functionality #92560

Merged

13 tasks

byronhulcher approved these changes Mar 1, 2021

View reviewed changes

cee-chen mentioned this pull request Mar 1, 2021

[App Search] Migrate Create Meta Engine View #92127

Merged

6 tasks

JasonStoltz self-assigned this Mar 2, 2021

JasonStoltz reviewed Mar 2, 2021

View reviewed changes

JasonStoltz approved these changes Mar 2, 2021

View reviewed changes

cee-chen force-pushed the app-logic branch from a2feeda to 1fd7c40 Compare March 2, 2021 16:57

cee-chen added 6 commits March 2, 2021 09:17

[Misc cleanup] Move Access type to common

447144b

- it was being duplicated in server/check_access and InitialAppData + add mock access data to DEFAULT_INITIAL_APP_DATA + update server/ tests to account for access in DEFAULT_INITIAL_APP_DATA

Update AppSearchConfigured to pass props to AppLogic vs calling initi…

6d0d451

…alizeAppData + update tests to rerender a wrapper rather than doing {...DEFAULT_INITIAL_APP_DATA} repeatedly

Update AppLogic to set values from props rather than a listener

09d9b12

- main goal of this PR is to prevent the flash of state between mount and initializeX being called - note: I recommend turning off whitespace changes in the test file

Update type scenarios where AppLogic values were previously thought p…

f38d6ad

…otentially undefined - which is mostly just configuredLimits it looks like

[PR feedback] Type name

f6bd9be

cee-chen force-pushed the app-logic branch from 1fd7c40 to f6bd9be Compare March 2, 2021 17:17

cee-chen merged commit fd3b3eb into elastic:master Mar 2, 2021

cee-chen deleted the app-logic branch March 2, 2021 19:25

kibanamachine mentioned this pull request Mar 2, 2021

[7.x] [App Search] Refactor AppLogic to initialize data via props rather than action (#92841) #93292

Merged

scottybollinger reviewed Mar 2, 2021

View reviewed changes

cee-chen mentioned this pull request Apr 24, 2021

[App Search] Fix error connecting state #98234

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Refactor AppLogic to initialize data via props rather than action #92841

[App Search] Refactor AppLogic to initialize data via props rather than action #92841

cee-chen commented Feb 25, 2021 •

edited

Loading

cee-chen commented Mar 1, 2021

byronhulcher left a comment

byronhulcher Mar 1, 2021

cee-chen Mar 1, 2021

cee-chen Mar 1, 2021

byronhulcher Mar 1, 2021

cee-chen Mar 1, 2021

JasonStoltz Mar 2, 2021 •

edited

Loading

cee-chen commented Mar 1, 2021 •

edited

Loading

JasonStoltz Mar 2, 2021

JasonStoltz left a comment

kibanamachine commented Mar 2, 2021

async chunk count

kibanamachine commented Mar 2, 2021

scottybollinger Mar 2, 2021

cee-chen Mar 2, 2021

scottybollinger Mar 2, 2021

		configuredLimits: [props.configuredLimits.appSearch, {}],
		ilmEnabled: [props.ilmEnabled, {}],

[App Search] Refactor AppLogic to initialize data via props rather than action #92841

[App Search] Refactor AppLogic to initialize data via props rather than action #92841

Conversation

cee-chen commented Feb 25, 2021 • edited Loading

Summary

Before

After

QA

Checklist

cee-chen commented Mar 1, 2021

byronhulcher 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

JasonStoltz Mar 2, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen commented Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

kibanamachine commented Mar 2, 2021

💚 Backport successful

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen commented Feb 25, 2021 •

edited

Loading

JasonStoltz Mar 2, 2021 •

edited

Loading

cee-chen commented Mar 1, 2021 •

edited

Loading