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

Search platform context cleanup #57448

Merged
merged 16 commits into from
Feb 19, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Feb 12, 2020

Summary

Resolves #54074

Removes the usage of core platform context and replaces it with getDependencies() along side some cleanup to bring the search service to a similar state as the other services in data plugin.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

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

@lizozom
Copy link
Contributor Author

lizozom commented Feb 12, 2020

@elasticmachine merge upstream

@lizozom lizozom marked this pull request as ready for review February 13, 2020 11:26
@lizozom lizozom requested a review from a team as a code owner February 13, 2020 11:26
@lizozom lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Feb 13, 2020
@lizozom
Copy link
Contributor Author

lizozom commented Feb 13, 2020

@elasticmachine merge upstream

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Overall this is looking really good, thanks for all of the cleanup! Other than what's listed below, I wanted to ask if it makes sense to move the types in the server/ folder into a dedicated types file like you've done with public/, or if you'd rather wait until we're modifying that code to do it.

examples/demo_search/public/index.ts Outdated Show resolved Hide resolved
examples/search_explorer/public/plugin.tsx Outdated Show resolved Hide resolved
src/plugins/data/public/search/search_service.ts Outdated Show resolved Hide resolved
src/plugins/data/public/search/types.ts Outdated Show resolved Hide resolved
*/

import { CoreStart } from 'kibana/public';
import { ISearch, ISearchGeneric } from './i_search';
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason you didn't move ISearch and ISearchGeneric into this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because they are used in examples/search_explorer/public/search_api.tsx and I thought they might be important enough to stay there.

src/plugins/data/public/search/search_service.ts Outdated Show resolved Hide resolved
Remove empty file reference
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Pulled & tested locally and things seem to be working! Thanks!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@lukasolson lukasolson merged commit 1319b65 into elastic:master Feb 19, 2020
lizozom pushed a commit to lizozom/kibana that referenced this pull request Feb 19, 2020
* Initial commit - cleanup

* cleanup

* tsing

* ts fixes

* Fix jest test

* Code review fxes

* Remove empty file reference

Remove empty file reference

* code review

Co-authored-by: Elastic Machine <[email protected]>
lizozom pushed a commit that referenced this pull request Feb 19, 2020
* Initial commit - cleanup

* cleanup

* tsing

* ts fixes

* Fix jest test

* Code review fxes

* Remove empty file reference

Remove empty file reference

* code review

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Search service] Remove usage of application mount context
4 participants