Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enterprise Search] Remove all instances of KibanaContext to Kea store #78513

Merged
merged 9 commits into from
Sep 28, 2020

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Sep 24, 2020

Summary

This is it, y'all!!! We've reached the endgame (well, there's still one more PR after this that handles sharing our store between our main app and our header app, but that won't require two teams to approve :)

Wow, such kea. much slim

Changelog

  • KibanaLogic now stores the following Kibana APIs:
    • config (not an API and doesn't update dynamically, but it seemed to fall under the general "comes from Kibana" bucket)
    • navigateToUrl
    • setBreadcrumbs
    • setDocTitle
    • (future: setHeaderActionMenu - this will be in the next PR 🎉)

As always, I recommend following along by commit! There's a lot of files touched + test refactors in there.

QA/Regression testing

  • Nothing crashes
  • Breadcrumb navigation should work as before
  • Document titles should still be set

Checklist

…seeffect

- The file is now no longer used to mock useContext, only unseEffect, hence the rename
- Remove mountWithKibanaContext completely
- Change mountWithContext to mountWithIntl, since that's the only context remaining, and move it to its own file
- Change mountWithAsyncContext to just mountAsync and move it to its own file
  + add an option to pair it w/ mountWithIntl for formatted date/number support
+ minor newline linting/grouping tweaks
@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 24, 2020
@cee-chen cee-chen requested review from a team September 24, 2020 22:02
Comment on lines +18 to +26
export const KibanaLogic = kea<MakeLogicType<IKibanaValues>>({
path: ['enterprise_search', 'kibana_logic'],
reducers: ({ props }) => ({
config: [props.config || {}, {}],
navigateToUrl: [props.navigateToUrl, {}],
setBreadcrumbs: [props.setBreadcrumbs, {}],
setDocTitle: [props.setDocTitle, {}],
}),
});
Copy link
Contributor Author

@cee-chen cee-chen Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the previous externalUrl PR, I also felt kinda weird that this store essentially just holds some variables that never update or require state, but in the end I decided to stay with a Kea logic file here because:

  1. It's familiar
  2. It's way easier to mock in tests since we already have the mocks/architecture set up

Admittedly those aren't like, super strong reasons heh, so if I'm definitely open to better paradigms if we think of them or if they come up down the road.

@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@scottybollinger scottybollinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so great! Thanks for doing it!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id value diff baseline
enterpriseSearch 249 +2 247

async chunks size

id value diff baseline
enterpriseSearch 472.5KB -16.8KB 489.2KB

page load bundle size

id value diff baseline
enterpriseSearch 21.7KB +3.0B 21.7KB

History

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


export const KibanaLogic = kea<MakeLogicType<IKibanaValues>>({
path: ['enterprise_search', 'kibana_logic'],
reducers: ({ props }) => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I didn't realize you could initialize logic with props like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! https://kea.js.org/docs/guide/additional/#props

Although as a warning (I think I mentioned this a few PRs back, but repeating it just as an fyi) if you set a value default to a passed prop without any fallbacks (e.g. someVar: [props.someVar, {}],), if you ever mount without passing a someVar prop the entire Kea/redux instance will error/crash. It's a good way of making sure you always have the data you need I guess? 😄

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@cee-chen cee-chen merged commit 3e426a0 into elastic:master Sep 28, 2020
@cee-chen cee-chen deleted the context-to-kea branch September 28, 2020 19:04
scottybollinger pushed a commit to scottybollinger/kibana that referenced this pull request Sep 28, 2020
elastic#78513)

* Add KibanaLogic store/mount

* Update usage of setBreadcrumbs/setDocTitle to KibanaLogic

* Update usage of Kibana config context to KibanaLogic

* Update usage of navigateToUrl context to KibanaLogic

* 🔥 Remove unused shallow_usecontext mocks, repurpose file to shallow_useeffect

- The file is now no longer used to mock useContext, only unseEffect, hence the rename

* 🔥 Delete/repurpose mount with context helpers

- Remove mountWithKibanaContext completely
- Change mountWithContext to mountWithIntl, since that's the only context remaining, and move it to its own file
- Change mountWithAsyncContext to just mountAsync and move it to its own file
  + add an option to pair it w/ mountWithIntl for formatted date/number support

* 🔥 Delete KibanaContext and mockKibanaContext

+ minor newline linting/grouping tweaks

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
cee-chen pushed a commit that referenced this pull request Sep 28, 2020
#78513) (#78692)

* Add KibanaLogic store/mount

* Update usage of setBreadcrumbs/setDocTitle to KibanaLogic

* Update usage of Kibana config context to KibanaLogic

* Update usage of navigateToUrl context to KibanaLogic

* 🔥 Remove unused shallow_usecontext mocks, repurpose file to shallow_useeffect

- The file is now no longer used to mock useContext, only unseEffect, hence the rename

* 🔥 Delete/repurpose mount with context helpers

- Remove mountWithKibanaContext completely
- Change mountWithContext to mountWithIntl, since that's the only context remaining, and move it to its own file
- Change mountWithAsyncContext to just mountAsync and move it to its own file
  + add an option to pair it w/ mountWithIntl for formatted date/number support

* 🔥 Delete KibanaContext and mockKibanaContext

+ minor newline linting/grouping tweaks

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
phillipb added a commit to phillipb/kibana that referenced this pull request Sep 29, 2020
…a into add-anomalies-to-timeline

* 'add-anomalies-to-timeline' of github.com:phillipb/kibana: (89 commits)
  Aligns several module versions across the repository (elastic#78327)
  Empty prompt and loading spinner for service map (elastic#78382)
  Change progress bar to spinner (elastic#78460)
  [QA][Code Coverage] Coverage teams lookup w/o Additional Config (elastic#77111)
  Slim down core bundle (elastic#75912)
  [Alerting] retry internal OCC calls within alertsClient (elastic#77838)
  [kbn/optimizer] only build xpack examples when building xpack plugins (elastic#78656)
  [Ingest Manager] Ingest setup upgrade (elastic#78081)
  [Ingest Manager] Surface saved object client 10,000 limitation to bulk actions UI (elastic#78520)
  fix name without a category or if field end with .text (elastic#78655)
  [Security Solution] [Detections] Log message enhancements (elastic#78429)
  [ML]DF Analytics exploration: default filter of results page by `defaultIsTraining` value in url (elastic#78303)
  [Enterprise Search] Remove all instances of KibanaContext to Kea store (elastic#78513)
  [ML] DF Analytics creation: ensure job did not fail to start before showing results link (elastic#78200)
  fix createAppNavigationHandler to use `navigateToUrl` (elastic#78583)
  Fixing a11y test failure on discover app (elastic#59975) (elastic#77614)
  [Security Solution] Initiate endpoint package upgrade from security app (elastic#77498)
  [kbn/es] use a basic build process (elastic#78090)
  [kbn/optimizer] fix .json extension handling (elastic#78524)
  Fix APM lodash imports (elastic#78438)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants