Move search bar and query bar to data plugin

[lizozom]

Summary

Moved search bar and query bar to data plugin.

I would love to get your feedback on how the components \ directives \ imports work now

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] This was checked for keyboard-only and screenreader accessibility

[elasticmachine]

Pinging @elastic/kibana-app-arch

[snide]

SCSS changes are just from the file moves. Looks good for us.

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[lukeelmers]

🎊 Exciting to see this coming together!

I haven't gotten to test yet, but looked at all the code and added some comments for further discussion.

Also, we should align on what the service names should be here. I think it makes sense for QueryBar to be part of a broader query service as we originally planned. (Query service would own query_manager, query_bar, and autocomplete_providers, and probably some other stuff).

And similarly whenever we move FilterBar, it could be part of a broader filter service.

But I'm not sure how we should deal with SearchBar. It's basically managing some state and wrapping query/filter bars. It doesn't feel like a dedicated service, as the only thing it is exporting is a React component. Maybe we should make a search service for the courier rewrite, and SearchBar could live alongside it?

[lukeelmers]

I don't know a lot about ui/modules, but if we are already updating the module names, would it make sense to scope them in a more granular fashion so it's easier to know what-depends-on-what later? e.g. data/query, data/search, etc instead of having them all use the same module name

we probably also don't need app in the module name, doesn't make much sense in the context of this particular plugin

[lizozom]

I'm not sure it's worth investing time into angular code that will be removed soon anyway. What do you think?

[lukeelmers]

I guess what I meant was, will it make it easier to remove angular code later if it is scoped more granularly now, especially since we are already changing these names now?

Definitely not worth investing the time if it won't help us later -- I was only suggesting it if you think it will save us time trying to figure out what's up with all of the various app/data references later.

[lukeelmers]

So I assume these imports are here because this code needs the directives that are wrapped in setupDirective to have been called already?

In making this change we are losing the context that search_bar is actually what's required by this particular app. This could make life more difficult later when we go to tear out Angular, as it won't be as obvious what exactly the dependencies are... it could be anything in plugins/data. This is a case I hadn't considered before.

Thinking out loud; what if, instead of calling setupDirective() from within the particular service, we exported it as a function from each service (and only allowed it to be called once)?

// search_bar_service.ts
setup() {
  return {
    SearchBar,
    loadLegacyDirectives: _.once(setupDirective)
  }
}

// dashboard_app.js
import { data } from 'plugins/data';

data.search.loadLegacyDirectives();

The first module that calls loadLegacyDirectives() would actually ensure setupDirective() is executed, and subsequent calls would do nothing; just serve the purpose of making it clear exactly what the module depends on.

There are other ways we could go about this too -- I am mostly interested in what you think about how we can make these dependencies more explicit.

[lizozom]

I was also bothered by the implicit import here.
Love your suggestion!

However, there are going to be multiple loadLegacyDirectives in many files.
What do you think about a convenience method on the service itself?
It's still implicit about which plugins are initialized, but at least the initialization itself is explicit.

import { data } from 'plugins/data';
data.loadLegacyDirectives();

[lukeelmers]

Yeah that would work too - the only downside I can think of is that by making the services implicit, we'll just have to reverse-engineer all references to data.loadLegacyDirectives() later to figure out exactly what services' directives a particular module is relying on.

If we are already doing the work today of pulling directives into specific services and updating downstream imports, keeping everything explicit might save us from more work later (avoiding an "autoload" type situation where you don't know what a module's true dependencies are).

But I'd defer to you on this -- you've spent much more time dealing with various legacy angular directives at this point 😃

[lizozom]

I found a nice (and necessary) middle ground, which you can see in the filter bar PR
https://github.com/elastic/kibana/pull/35460/files#diff-23034bf5cdc67a7321dbfb230edacd84

[lukeelmers]

I think we should have clear namespaces in the contract to make it clear what services own what methods:

search: this.searchBar.setup(),
query: this.queryBar.setup(),

This would prevent confusion for users because instead of importing like this:

import { data } from 'plugins/data';
const { toUser, fromUser } = data.helpers; // helpers for what?

They could instead do:

import { data } from 'plugins/data';
const { toUser, fromUser } = data.queryBar.helpers;

And if we are concerned about a dev's experience in having to destructure services just to get a React component, we could create a components.ts file at the top level that re-exports components for devs to import:

// plugins/data/public/components.ts
export { QueryBar } from './query_bar/components/query_bar';
export { SearchBar } from './search_bar/components/search_bar';

// plugins/foo-plugin/public/setup.ts
import { QueryBar, SearchBar } from 'plugins/data/components';

[lukeelmers]

Also if we actually move query/search bar in this PR, we'll probably need to add some dev docs before merging so any 3rd party devs who might be relying on ui/search_bar and ui/query_bar will know where to find them.

[lukeelmers]

@lizozom this is going to be replaced by #35389 and #35390, correct? if so, should we just close this one?

[lizozom]

Split into 2 PRs.
Closing.