Skip to content

Use index patterns service inside data plugin (rather than importing from ui/public)#41639

Merged
lizozom merged 4 commits intoelastic:masterfrom
lizozom:newplatform/data-plugin/use-index-patterns
Jul 24, 2019
Merged

Use index patterns service inside data plugin (rather than importing from ui/public)#41639
lizozom merged 4 commits intoelastic:masterfrom
lizozom:newplatform/data-plugin/use-index-patterns

Conversation

@lizozom
Copy link
Contributor

@lizozom lizozom commented Jul 21, 2019

Summary

Use index patterns service inside data plugin (rather than importing from ui/public)

Checklist

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

For maintainers

@lizozom lizozom added v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Jul 21, 2019
@lizozom lizozom self-assigned this Jul 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@lizozom lizozom added the review label Jul 21, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@lizozom lizozom mentioned this pull request Jul 22, 2019
4 tasks
@lizozom lizozom requested review from a team and ppisljar July 22, 2019 18:18
Copy link
Contributor

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

Another way you could do this would be importing from the top level (../../public) as mentioned in the IndexPatterns TS PR. That way we use the same "official" contracts between services that any other plugins would use:

// i want to import static code from another service within the data plugin
import { IndexPattern } from '../../../public';

// i want to use runtime dependencies from another service within the data plugin
class DataPlugin {
  setup(core, deps) {
    const indexPatterns = this.indexPatterns.setup();
    return {
      indexPatterns,
      myService: this.myService.setup({ indexPatterns })
    };
  }
}
MyService.setup({ indexPatterns });

// i want to use an internal method from another service & it is not publicly exposed from `data`
import { InternalThing } from '../../index_patterns';
// ^ makes it clear when we are relying on something internal to the data plugin

@lizozom
Copy link
Contributor Author

lizozom commented Jul 23, 2019

@lukeelmers I'm not sure how I feel about the static code import style, in a comlpex plugin like data.

Take this line for example:

https://github.com/elastic/kibana/pull/41639/files#diff-56d9e9ed6bea5cc4e4d3b79cddac2684R38

import { Field, IndexPattern } from '../../../index_patterns';

If we go with this approach, this line will become:

import { Field, IndexPattern } from '../../../';
  1. We're losing the ability to find which service the import comes from
  2. If we're importing things from multiple services, we're going to mix them in one long import.

What do you think?

@lukeelmers
Copy link
Contributor

I agree '../../../' is far more confusing, so in that case I'd go up another level to include the public directory: '../../../../public'. I don't feel strongly about this, was just bringing it up as an idea here.

I think this illustrates a larger decision we need to make which is how we organize our static exports by service to make them easier for devs to consume. That way you don't lose the context of which service you are dealing with when importing from public/index.

But for now, I think your approach importing from services ../../index_pattern works great and will be the better solution. It's all internal to our plugin anyway, so if we change our minds later we will have the flexibility to do this however we want.

@lizozom lizozom merged commit 87226e8 into elastic:master Jul 24, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Jul 24, 2019
…from ui/public) (elastic#41639)

* Use index patterns from data plugin itself instead of importing from ui/public

* Added mocks to field editor util tests
lizozom pushed a commit that referenced this pull request Jul 24, 2019
…from ui/public) (#41639) (#41867)

* Use index patterns from data plugin itself instead of importing from ui/public

* Added mocks to field editor util tests
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 24, 2019
* upstream/7.x:
  Ensure visualizations in percentage mode respect the advanced settings percent format (elastic#39044) (elastic#41855)
  [7.x] [telemetry] Analytics Package (elastic#41113) (elastic#41774)
  [Uptime] Improve `useUrlParams` hook for Uptime app (elastic#41545) (elastic#41818)
  [Maps] refactor filter actions to use embeddable actions (elastic#41713) (elastic#41821)
  [Maps] clean up tooltip header and footer (elastic#41793) (elastic#41816)
  [SIEM] Timeline NOT working with unauthorized  (elastic#41767) (elastic#41873)
  Use index patterns service inside data plugin (rather than importing from ui/public) (elastic#41639) (elastic#41867)
@lizozom lizozom deleted the newplatform/data-plugin/use-index-patterns branch November 14, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants