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

Remove circular dependency between data & expressions plugins #80510

Closed
mshustov opened this issue Oct 14, 2020 · 3 comments · Fixed by #82685
Closed

Remove circular dependency between data & expressions plugins #80510

mshustov opened this issue Oct 14, 2020 · 3 comments · Fixed by #82685
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) NeededFor:Core

Comments

@mshustov
Copy link
Contributor

mshustov commented Oct 14, 2020

data plugin lists expressions in requiredPlugins, while expressions declares data plugin as requiredBundles.
We need to remove circular dependency to unblock plugin migration to TS project refs #80508 and migration to the new build chain #69706

@elasticmachine
Copy link
Contributor

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

@mattkime mattkime added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Oct 14, 2020
@lukeelmers
Copy link
Member

lukeelmers commented Oct 15, 2020

data definitely needs the runtime dependency on expressions, so the question is what items from data is expressions using? Here's the list of what I could find:

  1. toPromise -- helper for AbortHandler
    • Used in the executor... I can't even find where it is used in data
  2. uniqFilters -- helper for deduping filters
    • Used in kibana_context
  3. Query Filter and TimeRange interfaces
    • Used in the ExpressionLoader's ExecutionContextSearch
    • Used in kibana_context

One idea for resolving this: if we were to move toPromise to expressions, and kibana_context to data, we could solve 1, 2, and part of 3 above.

The remaining problem would be the use of the Query, Filter and TimeRange interfaces in the expression loader. These are likely to cause issues in the future elsewhere as well as they are quite widely used.

Which leads me to another possible solution: way back when the expressions service was introduced, I had advocated for having it live inside data. If we wanted to revisit that discussion, it would be another way to solve the circular dependency. expressions has very few dependencies so we wouldn't need to add any new deps to data to make this work... something to think about. WDYT @ppisljar ?

@ppisljar
Copy link
Member

ppisljar commented Nov 2, 2020

lets cleanup expression loader as it shouldn't be using those types most likely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) NeededFor:Core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants