Skip to content

[Data Plugin]: Remove export * for common code from public/server index files#52821

Merged
lukeelmers merged 3 commits intoelastic:masterfrom
lukeelmers:fix/data-index-exports
Dec 12, 2019
Merged

[Data Plugin]: Remove export * for common code from public/server index files#52821
lukeelmers merged 3 commits intoelastic:masterfrom
lukeelmers:fix/data-index-exports

Conversation

@lukeelmers
Copy link
Copy Markdown
Contributor

After multiple discussions, including a recent mention in #52374, we determined that as an app architecture team convention, we should be explicitly exporting static code & types from the data/public/index.ts and data/server/index.ts.

This is important for two main reasons:

  1. Prevents leaking internal APIs which are simply implementation details of the plugin.
    • Devs will need to go out of their way to expose something on the public contract... which is a good thing.
    • Allows us to use export * in the rest of the plugin with confidence that we won't be messing with the public API.
  2. Creates a single file for someone to view the entire contract for a plugin's static code.
    • Helps with maintenance: when you see all of the interfaces that other plugins could be consuming in your plugin, it is easier to understand how widespread usage is.
    • Improves discoverability for devs using our plugins since they can "see" everything in one place.
    • Removes another level of indirection.

This PR takes a first step toward achieving these goals in the data plugin by doing the following:

  • Removes export * from '../common', which is one area where we had a lot of mixed internal/public code, some of which was accidentally leaking into the public contract.
  • Explicitly exports only the items from common which are used outside of the data plugin.
  • Removes the utils namespace inside of common/es_query which was introduced in Move @kbn/es-query into data plugin - es-query folder #50182. These could lead to confusion since the utils are specifically related to es_query and are mostly internal, yet were being exported from the top-level data/public and data/index.

Eventually the remaining data exports should also be cleaned up, as well as other plugins managed by the app arch team.

@lukeelmers lukeelmers requested a review from a team December 11, 2019 21:28
@lukeelmers lukeelmers self-assigned this Dec 11, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@lukeelmers lukeelmers force-pushed the fix/data-index-exports branch from e67a0c8 to 88f1543 Compare December 11, 2019 22:21
@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

History

  • 💔 Build #14968 failed e67a0c8f31d211e7e9ed81c65c47d625b4eef005

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Looks great!
LGTM

@lukeelmers lukeelmers removed the review label Dec 12, 2019
@lukeelmers lukeelmers merged commit 2caf640 into elastic:master Dec 12, 2019
@lukeelmers lukeelmers deleted the fix/data-index-exports branch December 12, 2019 15:00
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 30, 2019
…aved-objects

* upstream/master: (134 commits)
  [Dashboard] Add visualization from dasbhoard empty screen (elastic#52670)
  Print out agent debugging links during CI (elastic#52812)
  Add babel-plugin-styled-components to webpack config (elastic#52862)
  [Console] Fix load from remote (elastic#52814)
  Ensure APM agent config file path respects CWD (elastic#52880)
  [Watcher] Removed overwritten property (elastic#49998)
  [Data Plugin]: Remove `export *` for common code from public/server index files (elastic#52821)
  Hide stderr git output during APM agent configuration (elastic#52878)
  Polish migration.md (elastic#52764)
  Change ajax_stream to use new-line delimited JSON (elastic#52797)
  Stabilize dashboard save modal functional test (elastic#52761)
  [Discover] Place tooltip at bottom of filter button (elastic#52720)
  Disable/enable filter with click+shift on a filter badge (elastic#52751)
  [APM] Make client-side routes static (elastic#52574)
  [Maps] Get basic structure of NP client shim in place (elastic#52551)
  update chromedriver to 79 (elastic#52784)
  [DOCS] Adds example of assigning roles in Reporting (elastic#52757)
  Add instructions for setting up remote clusters needed for CCS and CCR (elastic#52796)
  [docs] max-old-space-size (elastic#52310)
  [Monitoring] Fix 7.5 cloud test issues (elastic#51781)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants