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

[DISCUSS] Moving @kbn/es-query into data plugin #42012

Closed
lukeelmers opened this issue Jul 25, 2019 · 4 comments
Closed

[DISCUSS] Moving @kbn/es-query into data plugin #42012

lukeelmers opened this issue Jul 25, 2019 · 4 comments
Labels

Comments

@lukeelmers
Copy link
Member

Summary

Recently as we've been converting Index Patterns to TypeScript, we discovered that the IndexPattern and Field interfaces which exist alongside the index patterns service are duplicated in packages/kbn-es-query.

The reason for this duplication is that packages can only import from other packages, so it isn't possible to relatively import a type interface from the data plugin (which houses the index patterns service) into kbn-es-query:

// packages/kbn-es-query/src/filters/index.d.ts

// breaks CI
import { Field, IndexPattern } from '../../../../src/core_plugins/data/public';

// works fine
import { Field, IndexPattern } from 'foo-package';

How should we deal with this? A few options come to mind:

1. Do nothing; keep duplicate interfaces in kbn-es-query

This is the path of least resistance, but would present challenges in terms of maintenance (what if the interface changes later and kbn-es-query continues to compile because we forgot to update it in two places?)

2. Move the interfaces to kbn-es-query

This would "fix" the problem, but the downside is that now we have a package owning these interfaces which arguably shouldn't be (they belong in the data plugin with index patterns). I suppose we could re-export them from the plugin if we want, but it still feels weird to me.

3. A dedicated package for types

Again, it works, but I think this goes against the whole idea of the new platform having clearly separated "domains" of ownership.

4. Move kbn-es-query into the data plugin

We already are doing static exports from the data plugin; why not move this package into it and export the public methods from there instead? Then there's no type sharing issues to worry about, and as a bonus we don't need to manage the build configuration for the package.


As you can probably tell from my descriptions above, I'm most in favor of option 4 😄
But would like to get input from others, especially as I have not spent as much time with this package and I'm not sure of all of the reasons for it being a package in the first place. (I'm guessing it was mostly due to a lack of better/easier options in the legacy world).

Lastly, while this question arose specifically in the context of es-query and index patterns, it raises a broader question: How do we deal with interfaces that need to be shared between plugins and packages?

We already know that the code in our packages needs to be static and not rely on any plugins, but type interfaces are an interesting exception since they are compiled away and I don't think it will be entirely unheard of for packages to make use of interfaces that are exported from plugins. (Maybe this is just another reason to avoid packages)

To be clear, the goal of this discussion is mainly to agree on what to do about es-query and index patterns. But would still be interested in other thoughts folks have on the issue more generally.

cc @Bargs @timroes @joshdover

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@Bargs
Copy link
Contributor

Bargs commented Jul 25, 2019

Can code in the data plugin be used on both the client and server? As far as I can remember, this was the main reason for creating the package in the first place. At the time there wasn't a better place to put a static library that needed to be used on client and server. @lukasolson might remember better if there were any other reasons for creating it.

@lizozom
Copy link
Contributor

lizozom commented Aug 1, 2019

According to the migration guide,

... can do so by exporting in the index files for both the server and public directories

So we would have to host the types in a common folder and re-export them twice.
What do you think?


I also looked at the functions exported by the plugin, and for example functions like buildExistsFilter would be natural to export from the data plugin, while getTimeZoneFromSettings could be exported from kibana_utils.

@lukeelmers
Copy link
Member Author

lukeelmers commented Aug 7, 2019

Closing; we agreed today that we would move forward with migrating @kbn/es-query into the data plugin (option 4), barring any technical issues.

A new issue has been opened to track this effort: #42885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants