[Enterprise Search] Move http out of React Context and to Kea Logic; prefer directly mounting #78167

cee-chen · 2020-09-22T15:59:26Z

Summary

This PR contains 2 refactors:

Part 1 of our migration away from React Context to Kea Logic, starting with the http lib (since it already exists in Kea via HttpLogic). We're making this technical choice for the following reasons:
- React Context was great during the original MVP plugin when we didn't need a lot of shared state, but I think we've outgrown it and need to move fully to Kea at this point.
- One reason is that we have to use a lot of native Kibana APIs directly within our Kea Logic files (examples: the http lib, the navigateToUrl method), and it's much easier usage-wise to store these in Kea directly from the get-go.
- The other reason is that moving everything to Kea also allows us to share Kea/Redux stores between separate React apps. This is now relevant because the Kibana header menu actions is technically a completely separate app, and as such it's impossible to share our current React contexts between them without passing duplicate props.

Relevant commits: [c69103c, c76735f, efe4f3d, 3653a23, & b763147]

Moving away from our LogicProvider React components, and instead preferring to mount logic files directly with props from our renderApp function.
@byronhulcher brought this up back in [Enterprise Search] Add reusable FlashMessages helper #75901 (when I added <FlashMessagesProvider />), and I think this was the implementation he was suggesting (although please let me know if I missed the mark). What this change does is:
- Removes the need for our initializeWhatever actions
- Ensures that certain required values/props (in this case - the http dependency) is loaded into the logic file as soon as possible. Essentially with our initializeWhatever actions, there was a small period where state was being set where values would flash between null (or whatever default) and the passed prop. By setting value defaults to props.whatever:
  - We load values in immediately to logic files - no state changes, and as such fewer rerenders
  - Removes defaults for any values/dependencies that are crucial - if the logic is mounted incorrectly or if the prop is missing or undefined, Redux will just crash :) A bit drastic admittedly, but allows us to write code knowing for sure we can depend on on certain values (a discussion we'd previously had here in a prior PR).
- Decouples mounting our logic from React lifecycles (less uncertainty, fewer extra files/components)

Relevant commits: [16f9c73 & 2d13707]

This PR touches a lot of files and contains test refactors as well - I strongly recommend following along by commit. I hopefully left some semi-helpful commit messages explaining my process & thinking as well.

QA/Regression testing

HTTP calls (to our API endpoints, to telemetry actions) still 200/work as expected
HTTP listeners work as expected:
- Changing to read-only mode and then navigating between AS & ES plugins
- Stop Enterprise Search and navigate between plugins, error connecting message should pop up
Flash Messages still work as before (requires modifying some code directly to test)

Checklist

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

cee-chen · 2020-09-22T16:00:32Z

Oops dang I hit ctrl+enter and that created the PR before I was ready to open it. Will edit the description and fix merge conflicts in a bit

- removes need for initializeHttp call - ensures http value is loaded into HttpLogic as soon as possible / should never load in as null, reducing # of rerenders/checks see: https://kea.js.org/docs/guide/advanced#mounting-and-unmounting

- Kea mock import should now contain mock default values which can be overridden

send_telemetry: - refactor to use shallow (w/ useEffect mocked) vs mount - check mockHttpValues directly engine_table: - refactor to use mount w/ an I18nProvider rather than mountWithContext helper (which we'll likely need to overhaul in the future) - assert mockHttpValues directly

@scottybollinger

EngineOverview: - Change use of FormattedMessage to i18n.translate (simpler, no provider required) Tests: - Create mock values/actions for FlashMessages, since EngineOverview calls it - Create combined mockAllValues obj for easier overriding - Create setMockValues helper for easier test overriding (credit to @scottybollinger for the idea!) - Update engine_overview tests to setMockValues instead of passing context to mountWithAsyncContext - Fix mountWithAsyncContext to accept an undefined obj

- it should now only live in HttpLogic 🔥

@JasonStoltz

…rops - send history as prop - refactor out now-unnecessary listenToHistory (we can just do it directly in afterMount without worrying about duplicate react rerenders) - add mount helper Tests: - refactor history.listen mock to mockHistory (so that set_message_helpers can use it as well) - use mountFlashMessagesLogic + create an even shorter mount() helper (credit to @JasonStoltz for the idea!) - refactor out DEFAULT_VALUES since we're not really using it anywhere else in the file, and it's not super applicable to this store - update history listener tests to account for logic occurring immediately on mount

cee-chen · 2020-09-22T16:18:17Z

OK, I think this PR is ready for review! 🙇‍♀️ It touches a lot of files and contains test refactors, so I strongly recommend following along by commit. I left (hopefully) helpful commit messages explaining my process & thinking throughout.

cee-chen · 2020-09-22T16:29:22Z

Something I want to clarify as well - for 2., I'm not suggesting we should move to using props for any of our upcoming migrated views or components, just for shared dependencies/libs that need to be stored at plugin init (i.e., only for logic files being instantiated at renderApp in the first place).

Ideally at the end of this series of refactors, our final renderApp JSX should look like this:

<I18nProvider>
  <Provider store={store}>
    <Router history={params.history}>
      <App {...initialData} />
    </Router>
  </Provider>
</I18nProvider>

... with all our logic/context initialization being handled above ReactDOM.render() as standard JS.

Nice and clean, right??

scottybollinger

LGTM! Thanks for the super clean commit history!

cee-chen · 2020-09-22T16:47:28Z

x-pack/plugins/enterprise_search/public/applications/__mocks__/kea.mock.ts

+import { useValues } from 'kea';
+
+export const setMockValues = (values: object) => {
+ (useValues as jest.Mock).mockImplementation(() => ({ ...mockAllValues, ...values }));
+};


Oh, I totally forgot to give you credit for this @scottybollinger! I stole this from your awesome overview mocks 😁

kibanamachine · 2020-09-22T17:50:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2d13707

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
enterpriseSearch	275	-2	277

async chunks size

id	value	diff	baseline
enterpriseSearch	510.2KB	-2.6KB	512.8KB

History

💔 Build #76227 failed 0d8aa4e

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

byronhulcher

This looks in order, I'm glad to see this using our Kea Logic pattern vs the provider pattern in place

scottybollinger · 2020-09-22T18:15:42Z

@byronhulcher can we get a ✅ if everything looks good?

cee-chen · 2020-09-22T19:33:32Z

Thanks y'all!! 🍕

@scottybollinger

…prefer directly mounting (#78167) (#78204) * Remove HttpProvider in favor of mounting HttpLogic directly w/ props - removes need for initializeHttp call - ensures http value is loaded into HttpLogic as soon as possible / should never load in as null, reducing # of rerenders/checks see: https://kea.js.org/docs/guide/advanced#mounting-and-unmounting * Update simplest components using http for sendTelemetry * Update simplest tests for components using HttpLogic + default Kea mocks - Kea mock import should now contain mock default values which can be overridden * Update moderately complex tests using HttpLogic send_telemetry: - refactor to use shallow (w/ useEffect mocked) vs mount - check mockHttpValues directly engine_table: - refactor to use mount w/ an I18nProvider rather than mountWithContext helper (which we'll likely need to overhaul in the future) - assert mockHttpValues directly * Update EngineOverview to HttpLogic + refactors EngineOverview: - Change use of FormattedMessage to i18n.translate (simpler, no provider required) Tests: - Create mock values/actions for FlashMessages, since EngineOverview calls it - Create combined mockAllValues obj for easier overriding - Create setMockValues helper for easier test overriding (credit to @scottybollinger for the idea!) - Update engine_overview tests to setMockValues instead of passing context to mountWithAsyncContext - Fix mountWithAsyncContext to accept an undefined obj * Remove http from KibanaContext - it should now only live in HttpLogic 🔥 * Remove FlashMessagesProvider in favor of mounting logic directly w/ props - send history as prop - refactor out now-unnecessary listenToHistory (we can just do it directly in afterMount without worrying about duplicate react rerenders) - add mount helper Tests: - refactor history.listen mock to mockHistory (so that set_message_helpers can use it as well) - use mountFlashMessagesLogic + create an even shorter mount() helper (credit to @JasonStoltz for the idea!) - refactor out DEFAULT_VALUES since we're not really using it anywhere else in the file, and it's not super applicable to this store - update history listener tests to account for logic occurring immediately on mount

cee-chen requested review from a team September 22, 2020 15:59

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 22, 2020

cee-chen added 7 commits September 22, 2020 09:11

Update simplest components using http for sendTelemetry

c69103c

Update simplest tests for components using HttpLogic + default Kea mocks

c76735f

- Kea mock import should now contain mock default values which can be overridden

Remove http from KibanaContext

b763147

- it should now only live in HttpLogic 🔥

cee-chen force-pushed the context-to-kea branch from 0d8aa4e to 2d13707 Compare September 22, 2020 16:15

cee-chen requested a review from byronhulcher September 22, 2020 16:17

cee-chen requested a review from scottybollinger September 22, 2020 16:18

scottybollinger approved these changes Sep 22, 2020

View reviewed changes

cee-chen commented Sep 22, 2020

View reviewed changes

byronhulcher reviewed Sep 22, 2020

View reviewed changes

byronhulcher approved these changes Sep 22, 2020

View reviewed changes

cee-chen merged commit 42026cb into elastic:master Sep 22, 2020

cee-chen deleted the context-to-kea branch September 22, 2020 19:33

cee-chen mentioned this pull request Sep 22, 2020

[7.x] [Enterprise Search] Move http out of React Context and to Kea Logic; prefer directly mounting (#78167) #78204

Merged

cee-chen mentioned this pull request Sep 22, 2020

[Enterprise Search] Move LicenseContext to Kea #78231

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Move http out of React Context and to Kea Logic; prefer directly mounting #78167

[Enterprise Search] Move http out of React Context and to Kea Logic; prefer directly mounting #78167

cee-chen commented Sep 22, 2020 •

edited

Loading

cee-chen commented Sep 22, 2020 •

edited

Loading

cee-chen commented Sep 22, 2020

cee-chen commented Sep 22, 2020 •

edited

Loading

scottybollinger left a comment

cee-chen Sep 22, 2020 •

edited

Loading

kibanamachine commented Sep 22, 2020

byronhulcher left a comment

scottybollinger commented Sep 22, 2020

cee-chen commented Sep 22, 2020

[Enterprise Search] Move http out of React Context and to Kea Logic; prefer directly mounting #78167

[Enterprise Search] Move http out of React Context and to Kea Logic; prefer directly mounting #78167

Conversation

cee-chen commented Sep 22, 2020 • edited Loading

Summary

QA/Regression testing

Checklist

cee-chen commented Sep 22, 2020 • edited Loading

cee-chen commented Sep 22, 2020

cee-chen commented Sep 22, 2020 • edited Loading

scottybollinger left a comment

Choose a reason for hiding this comment

cee-chen Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Sep 22, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

History

byronhulcher left a comment

Choose a reason for hiding this comment

scottybollinger commented Sep 22, 2020

cee-chen commented Sep 22, 2020

cee-chen commented Sep 22, 2020 •

edited

Loading

cee-chen commented Sep 22, 2020 •

edited

Loading

cee-chen commented Sep 22, 2020 •

edited

Loading

cee-chen Sep 22, 2020 •

edited

Loading