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

move agg_types and agg_configs to data plugin #41878

Closed
wants to merge 9 commits into from

Conversation

ppisljar
Copy link
Member

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

I've only looked at the initial directory structure. A few thoughts:

  • we could take this opportunity to convert setup.ts to legacy.ts (the agreed-upon convention for shim code). since you are already updating a bunch of imports anyway, this would prevent you from needing to do them again...
  • are we planning to statically import all of the agg_types/agg_configs code? or should we have an aggs service, e.g. data.aggs.types and data.aggs.configs or similar?
  • even if we are only doing static imports, i might still advocate for doing them under a single aggs directory
  • for the static imports, we should probably still be exporting those from the top-level public/index.ts rather than doing our imports from public/np_ready. or perhaps you were doing this as a temporary step?
  • i have some ideas for how we could handle exporting the static stuff a bit more cleanly; will bring this up tomorrow

@ppisljar ppisljar closed this Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants