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

[data.search.SearchSource] Extract dependencies and pass them in from the service definition. #75368

Merged
merged 7 commits into from
Aug 20, 2020

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Aug 18, 2020

Part of #65069

Summary

This PR does the following:

  • Removes some service getters in services.ts that are no longer needed
  • Removes injectedMetadata dependency from SearchSource, and instead explicitly passes the necessary values in.
  • Creates a common GetConfigFn interface which serves as a synchronous wrapper for uiSettings.get
    • We had the same function implemented in field formats, aggs, and search source, so this simply updates them all to use the same type
  • Removes all SearchSource usages of uiSettings, instead switching them over to use getConfig
  • Fixes a few circular dependencies that surfaced in common/field_formats

Testing

This introduces no functional changes as it is only a refactor. Areas of the UI to check for regressions include Discover, Visualize (including Vega), and Lens.

Dev Docs

The search service's getParamsFromSearchRequest helper has been changed to prepare for exposing SearchSource on the server. If your plugin relies on this helper, please update the dependencies passed to it as follows:

       import { getSearchParamsFromRequest } from '../../../src/plugins/data/public';

       const params = getSearchParamsFromRequest(request, {
-          injectedMetadata: core.injectedMetadata,
-          uiSettings: core.uiSettings,
+          esShardTimeout: core.injectedMetadata.getInjectedVar('esShardTimeout') as number,
+          getConfig: core.uiSettings.get.bind(core.uiSettings),
       });

@lukeelmers

This comment has been minimized.

@lukeelmers lukeelmers self-assigned this Aug 18, 2020
@lukeelmers lukeelmers added Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 18, 2020
@lukeelmers lukeelmers force-pushed the fix/ssource-dependencies branch from 606273f to c336dd1 Compare August 18, 2020 23:08
@lukeelmers lukeelmers marked this pull request as ready for review August 18, 2020 23:09
@lukeelmers lukeelmers requested a review from a team August 18, 2020 23:09
@lukeelmers lukeelmers requested a review from a team as a code owner August 18, 2020 23:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers lukeelmers force-pushed the fix/ssource-dependencies branch from c336dd1 to ae56af4 Compare August 18, 2020 23:12
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@lukeelmers lukeelmers force-pushed the fix/ssource-dependencies branch from ae56af4 to 81feb10 Compare August 19, 2020 19:55
@lukeelmers

This comment has been minimized.

1 similar comment
@lukeelmers

This comment has been minimized.

@lukeelmers
Copy link
Member Author

retest

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, CodeOwner review, checked out locally, tested in Firefox, works.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
data 1.4MB -499.0B 1.4MB
visTypeVega 661.9KB +70.0B 661.8KB
total -429.0B

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants