[data.search.aggs] Remove service getters from agg types #61069
Conversation
alexwizp
added
v7.7.0
v8.0.0
Feature:NP Migration
Feature:Aggregations
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
alexwizp
added
the
release_note:skip
| getDateHistogramBucketAgg(deps), | ||
| getHistogramBucketAgg(deps), | ||
| rangeBucketAgg, | ||
| getDateRangeBucketAgg(deps), | ||
| ipRangeBucketAgg, | ||
| termsBucketAgg, | ||
| filterBucketAgg, | ||
| getFiltersBucketAgg(deps), |
There was a problem hiding this comment.
I think we should pass only the required dependencies to each agg type, rather than AggTypesDependencies, which includes all of core and all of the query service. (Similar to how getFiltersBucketAgg was previously implemented)
Each of these agg types only uses a tiny fraction of each of these services -- for the sake of maintainability I would much rather be explicit about which service dependencies an agg type actually has... that way we don't have to go opening up every single agg type file in the future if we want to find out what the dependencies are.
Another reason for doing this is that we are eventually going to need to support aggs on the server, which makes it even more important that we aren't coupling these to the public CoreSetup, as this is already going to be different from the server CoreSetup.
So my suggestion would be:
- Only pass the required services into
getAggTypes - Only pass the required services into each individual agg (this should be a subset of what's provided to
getAggTypes
| export const createMockedAggTypesDependencies = (): AggTypesDependencies => ({ | ||
| core: coreMock.createSetup(), | ||
| query: queryServiceMock.createSetupContract(), | ||
| }); |
There was a problem hiding this comment.
If we go the route of only passing in the required deps as I described above, then we wouldn't need a test helper -- each individual agg could import only the mocks they need for their tests.
| @@ -410,7 +403,6 @@ export const npStart = { | |||
| search: { | |||
| aggs: { | |||
| calculateAutoTimeExpression: sinon.fake(), | |||
| createAggConfigs: sinon.fake(), | |||
There was a problem hiding this comment.
good catch, this was my bad 😄
|
@elasticmachine merge upstream |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
LGTM! Tested and everything seems to be functioning as expected. Thanks @alexwizp!
| return new Promise((resolve, reject) => { | ||
| if (!this.vis.type || !this.vis.type.visConfig || !this.vis.type.visConfig.component) { | ||
| reject('Missing component for ReactVisType'); | ||
| } |
There was a problem hiding this comment.
nit: this might be more succinct:
const Component = this.vis.type?.visConfig?.component;
if (!Component) {
reject('Missing component for ReactVisType');
}There was a problem hiding this comment.
@lukeelmers Have not idea why but ?. is not working with karma tests =(
* [data.search.aggs] Remove service getters from agg types Part of elastic#60333 * fix JEST * fix karma:unit * fix PR commnets * fix PR comments * try to fix ci * fix CI * fix karma:unit Co-authored-by: Elastic Machine <[email protected]>
…1582) * [data.search.aggs] Remove service getters from agg types Part of #60333 * fix JEST * fix karma:unit * fix PR commnets * fix PR comments * try to fix ci * fix CI * fix karma:unit Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Closes: #60333
Summary
When aggs were migrated to the new platform data.search service, each of the runtime dependencies in the individual agg types were retrieved using service getters/setters (e.g. getUiSettings or getFieldFormats in src/plugins/data/public/services)
Done for
Checklist
Delete any items that are not applicable to this PR.
For maintainers