TypeScript: AggregationsResult improvements

[edloidas]

Right now AggregationsResult type can include three different type of aggregations. There is no other way to differ them unless we check for known properties in aggregation objects:

if ('buckets' in aggregation) {
  // probably a BucketsAggregationResult
}

It will be much safer and robust to check for the type property that is currently missing.

The suggestion is to add type property to Java and TypeScript types, to make it better to use aggregations in server-side JS/TS:

if (aggregation.type === 'bucket') {
  // definitely a BucketsAggregationResult
}

[edloidas]

@tajakobsen @alansemenov @rymsha @anatol-sialitski

[rymsha]

If a developer created a query, then the type of aggregation(s) is known by its position. There should be no need to complicate the structure.

[tajakobsen]

This was initially my suggestion (so I'm biased here), and the reason for me wanting this is TypeScript-related.

It is true that the developer knows the type of aggregation, but TypeScript doen't know the it. TypeScript just knows that the result is one of the options in this union.

export type AggregationsResult = 
  | BucketsAggregationResult 
  | StatsAggregationResult 
  | SingleValueMetricAggregationResult;

Too difficult

I think the following approach is too hard to figure out for a new XP-developer:

if (aggregation.type === 'bucket') {
  // definitely a BucketsAggregationResult
}

They would probably just do a hack like this, and call it a day. (This would lead to brittle code)

const buggetAggregation = aggregation as unknown as BucketsAggregationResult;

The TypeScript-way

I think there are two options that can lead to good DX (Developer Experience).

1. Implement the union as a discriminating union, and let the developer use the type field (or another name) to check which type of aggregation it is. (the suggestion in this issue)
2. Make ContentsResult.aggregations a type parameter <A>. We have two options here:
   1. <A> can either be passed in as a type parameter to query()
   2. We can infer the shape of <A> based on QueryContentParams.aggregations. (I was doing something similar to this in the old days, when I used the <AggregationKeys> from the input to give the shape of the output).

[rymsha]

1 "Implies" a switch? Developers will not write a switch for 3 types of aggs, as they usually specify just one in a query.
2.i seem to be just a shorter version of a "hack" - casting via generic.
IMO only 2.ii sounds promising.

[tajakobsen]

I like all the options 🙂

---

1 "Implies" a switch?

Probably just an if-statement (or filter) most often. But it will give certainty of what the type is

2.i seem to be just a shorter version of a "hack" - casting via generic.

Using type constraints (with the extends keyword (e.g <A extends Record<string, Aggregation>) can give some guide rails here, but I don't disagree that this can be brittle too

2.ii sounds promising.

This is probably the coolest solution. But it would be the most complex TypeScript in the whole project.

But if you make this, it will feel like magic 🪄 for the developers (in a positive way)

[tajakobsen]

I looked into it, and I think 2.ii is impossible.

TypeScript does NOT allow "partial inferance of type parameters". So if <Hit> is defined, the second type parameter must also be defined.

And that second parameter must both contain the "name/key" of the aggregation, and input type or return type of the aggregation. And then we are at 2.i anyway.

Regarding this:

2.i seem to be just a shorter version of a "hack" - casting via generic.

By passing the shape of the output as a type parameter, we can do a typecheck on the input too.

So that we can invalidate an input shape that doesn't produce the specified output shape.

[rymsha]

2.i is the best option then, IMO. And as far as I understand it does not require to have new type field.

[tajakobsen]

And as far as I understand it does not require to have new type field.

That is correct

[tajakobsen]

It is also possible that the second type parameter could be gotten using typeof

const aggregations = {
  test: {
    terms: {
      field: "title",
    },
  },
} as const;

const res = query<Content<Article>, typeof aggregations>({
  aggregations
})

---

And the type parameter for aggregations might be be inferred if no type for <Hit extends Content> is provided (sometimes we are not interested in data, only aggregations)

[tajakobsen]

By passing the shape of the output as a type parameter, we can do a typecheck on the input too.

Actually it's probably better to pass in the input shape, because then we can either use the typeof approach above, or for cases where Content is not needed, it can infer the input shape itself.

We can pretty easily get the output shape from the input shape.

[tajakobsen]

@edloidas Here is my code for this, that takes you about 80% there.

import type {
  Aggregation,
  BucketsAggregationResult,
  Content,
  Filter,
  Highlight,
  HighlightResult,
  QueryDsl,
  SingleValueMetricAggregationResult,
  SortDsl,
  TermsAggregation,
  MinAggregation,
  AggregationsResult,
} from "@enonic-types/lib-content";

export interface QueryContentParams<AggregationInput extends Record<string, Aggregation> = never> {
  start?: number;
  count?: number;
  query?: QueryDsl | string;
  sort?: string | SortDsl | SortDsl[];
  filters?: Filter | Filter[];
  aggregations?: AggregationInput;
  contentTypes?: string[];
  highlight?: Highlight;
}

export interface ContentsResult<
  Hit extends Content<unknown>,
  AggregationOutput extends Record<string, AggregationsResult>
> {
  total: number;
  count: number;
  hits: Hit[];
  aggregations: AggregationOutput;
  highlight?: Record<string, HighlightResult>;
}

type AggregationInputToOutput<Type extends Aggregation> = Type extends TermsAggregation
  ? BucketsAggregationResult
  : Type extends MinAggregation
  ? SingleValueMetricAggregationResult
  : never;

export declare function query<
  Hit extends Content<unknown> = Content,
  AggregationInput extends Record<string, Aggregation> = never
>(
  params: QueryContentParams<AggregationInput>
): ContentsResult<
  Hit,
  {
    [Key in keyof AggregationInput]: AggregationInputToOutput<AggregationInput[Key]>;
  }
>;

const aggregations = {
  test: {
    terms: {
      field: "title",
    },
  },
  test2: {
    min: {
      field: "3",
    },
  },
};

const res = query<Content, typeof aggregations>({
  aggregations,
});

const checkThisType: {
  test: BucketsAggregationResult;
  test2: SingleValueMetricAggregationResult;
} = res.aggregations;

It needs some better handling of the case where ContentsResult.aggregations is undefined.

---

Regarding using this in JavaScript-land, it should automatically infer the correct type since no type parameters are passed in. So we don't need a "catch all" JavaScript case.

[tajakobsen]

Also remember to look into resolving NumericBucket or DateBucket in AggregationInputToOutput.

[tajakobsen]

I see that I have not taken into account nested aggregations. So that is something @edloidas can have fun with! 😄