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

Separate class\type definitions from plugin instance setup #38894

Merged

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Jun 13, 2019

Summary

I'm having a problem with circular imports in new platform shims, and I suspect this might happen more often as we start using shim plugins around the code.

For example, as QueryFilter now uses the data plugin, I'm getting the following dependency loop:
visualize/../run_pipeline -> plugins/interpreter ... -> filter_manager/query_filter -> plugins/data -> plugins/interpreter.

This PR separates class\type definitions from plugin instance setup in shim plugin definition.
I would recommend using this patterns in other shim plugins as well.

Usage

Now when you want to import any classes or types, you would import straight from the plugin

import { Query } from 'plugins/data';

And if you want the plugin instance, you would do so through the setup file:

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

…ugin definition

This helps avoiding circular dependency issues that were obsereved in filter-manager branch (due to code starting to use the data plugin).
@lizozom lizozom requested a review from a team June 13, 2019 15:08
@lizozom lizozom self-assigned this Jun 13, 2019
@lizozom lizozom added Feature:New Platform non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0 labels Jun 13, 2019
@lizozom lizozom requested review from ppisljar and streamich June 13, 2019 15:09
Copy link
Contributor

@streamich streamich 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.

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.

LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

LGTM

@lizozom lizozom merged commit 4a39c7a into elastic:master Jun 14, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Jun 15, 2019
…8894)

* Separate class\type defenitions from plugin instance setup in shim plugin definition
This helps avoiding circular dependency issues that were obsereved in filter-manager branch (due to code starting to use the data plugin).

* typescript fun
lizozom pushed a commit that referenced this pull request Jun 16, 2019
…39039)

* Separate class\type defenitions from plugin instance setup in shim plugin definition
This helps avoiding circular dependency issues that were obsereved in filter-manager branch (due to code starting to use the data plugin).

* typescript fun
@Bargs Bargs mentioned this pull request Jun 20, 2019
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants