Define and export type SearchResponse

[villasv]

🚀 Feature Proposal

Export the generic types defined in https://www.elastic.co/guide/en/elasticsearch/client/javascript-api/current/typescript.html (specially the SearchResponse<T>)

Motivation

This way people won't have to reimplement these types every time.

Example

The exact same documentation example, but we would only have to define the Source type.

[villasv]

I've seen the comment at #813 (comment) (emphasis mine)

Because there is not an official spec from which I can generate the response types, and it will make no sense export the types just for the search response.

I can see that it would be too much work to give types to all possible response types, and it would be inconsistent to provide a response type just for the search.

Even so, I believe it's better to have this. Currently, the API is already inconsistent in that a few places are typed and others don't. I'd rather have some response types than none.

[villasv]

My proposal: have a separate d.ts for types that are hand curated and warn about their incompleteness, so they can be exported like some kind of experimental types.

[delvedor]

Hello!

Currently, the API is already inconsistent in that a few places are typed and others don't. I'd rather have some response types than none.

Can you elaborate? What you mean by "inconsistent"?

My proposal: have a separate d.ts for types that are hand curated and warn about their incompleteness, so they can be exported like some kind of experimental types.

This could be a viable solution, even if I don't like the idea of exporting only a subset of typed responses. I'll think about it.

[lukeed]

Not having the API responses typed by default is pretty disappointing tbh – it's maybe half(?) the reason I wanted to use the definitions in the first place.

The documentation around using the library methods and/or constructing API requests is plentiful, but I have almost zero idea what the response will look like until I actually run the query.

Would love to see this change in the future.

[villasv]

@delvedor Can you elaborate? What you mean by "inconsistent"?

I'm sorry, after re-reading I now understand what you meant with inconsistent. I agree this is not a good situation. Having a half-baked type system might as well worse than not having one at all and I imagine many people will get frustrated, just like now. OTOH I think we can grow to a reasonably comprehensive type system quite fast if we count on the community to come here fill the blanks whenever they find some.

Right now at our company we have an es-types module that gets bigger by the day, but I wouldn't check that in because we add fields incrementally and that generates lots of changes. If we could add types and have confirmation that all fields have been taken into account, I think we can evolve this part of the type system to be useful even if not complete.

[lukeed]

You guys should change your types such that this:

search: ApiMethod<RequestParams.Search>

... becomes this:

search: ApiMethod<Search>
// or
search: ApiMethod<'Search'>
// or 
search: Search

I would prefer the last option, and its less verbose and is easier to find. The idea is that Search is an interface that encapsulates its own Params and Response shapes, all while implementing generics.

export interface SearchRequest<T=any> extends Generic {
  body?: T;
  // ...
  index?: string | string[];
  _source_exclude?: string | string[];
  _source_include?: string | string[];
  analyzer?: string;
}

export interface SearchResult<T=any> {
  _source: T;
  _index: string;
  _type: string;
  _id: string;
  _score: number;
  _version?: number;
  _explanation?: Explanation;
  fields?: any;
  highlight?: any;
  inner_hits?: any;
  matched_queries?: string[];
  sort?: string[];
}

 export interface SearchResponse<T=any> {
  took: number;
  timed_out: boolean;
  _scroll_id?: string;
  _shards: ShardsResponse;
  aggregations?: any;
  hits: {
    total: number;
    max_score: number;
    hits: SearchResult<T>[];
  };
}

export interface Search<Req=any, Res=any> {
  Request: SearchRequest<Req>;
  Response: SearchResponse<Res>;
}

I would rework the current transport layer to reduce some of the overhead here, but in the meantime, this could be done (per method -- generate ftw? 😅)

interface ApiMethod<TParams, TBody = any> {
  // Promise API
  (): Promise<ApiResponse<TBody>>;
  (params: TParams): Promise<ApiResponse<TBody>>;
  (params: TParams, options: TransportRequestOptions): Promise<ApiResponse<TBody>>;
  // Callback API...
}

// Extend ApiMethod per method
interface SearchApiMethod<X=any, Y=any> extends ApiMethod<Search<X>['Request'], Search<Y>['Response']> {
  // 
}

// or this, if you don't want the extra `Search` interface
interface SearchApiMethod<X=any, Y=any> extends ApiMethod<SearchRequest<X>, SearchResponse<Y>> {
  // 
}

Then the Client becomes this:

declare class Client extends EventEmitter {
  constructor(opts?: ClientOptions);
  connectionPool: ConnectionPool;
  transport: Transport;
  serializer: Serializer;
  extend: ClientExtends;
  child(opts?: ClientOptions): Client;
  close(callback?: Function): Promise<void> | void;

  /* GENERATED */
  // ...
  search: SearchApiMethod
  // ...
}

[mshustov]

The lack of a centralized type registry starts being a problem for Kibana as well.
Kibana still uses outdated @types/elasticsearch https://github.com/elastic/kibana/blob/69fc4437ff6ef227de14dd859a755dd11281973f/package.json#L323 which creates problems for developers elastic/kibana#56694
Probably we should extract elasticsearch API related type definitions to a separate library/repo that can be consumed by @elastic/elasticsearch and publish it to @types/elasticsearch as well. Or make @types/elasticsearch as a source of truth for the @elastic/elasticsearch. WDYT?

[delvedor]

@restrry I'm not sure what you mean by "centralized type registry".
This library gives out of the box all the types it can define, if something it's not present, it means that we can't generate it.

I'm still on the fence about publishing a handwritten definition of the responses, as there is no way to test their correctness.

[villasv]

Is there a backlog that describes how would we achieve the generation of those types that are still missing?

[joshdover]

I'm still on the fence about publishing a handwritten definition of the responses, as there is no way to test their correctness.

I think that until we have a real API spec from Elasticsearch that defines the full typings so we can generate all types, developers in the wild are going to end up writing types by hand anyways. I know in the case of Kibana, we will definitely want to fill in the gaps of what's missing.

So then the question becomes, why duplicate all this work in each project rather than having them done in one place? By having handwritten types in this package, all projects can benefit from more complete typings, even if they're not 100% perfect. I think we're much more likely to get it right if done here then if done in each downstream project that consumes this library.

In terms of where these types should live, I personally prefer they live in this module rather than in @types/elasticsearch. Primary benefit being that it's much more obvious if the full types ship with the library than if they're in some auxiliary package.

If that's the direction we go, I think we should also look into having @types/elasticsearch removed or at least marked as deprecated in some way.

[nreese]

I 100% agree with everything @joshdover said. Could the aggregation request types created here https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/typings/elasticsearch/aggregations.ts be used as a starting point to get this moving forward?

Progress over perfection is much preferred then having nothing at all.

[dgieselaar]

I think I was supposed to have a call about moving the APM typings into the ES client, but it dropped off my radar for a bit. Should we give that another go? Who wants/needs to join other than @delvedor?

[joshdover]

@delvedor and I met a few weeks back and agreed that we can / should allow manual typings in this project until the day that Elasticsearch provides a complete spec.

I believe at this point, pull requests are welcome. Work was started on this a while back in #970, but it probably needs a new owner to carry that over the line.

[delvedor]

Hello! Thank you @joshdover for the summary!
I've just opened #1119, which should vastly improve the developer experience with TypeScript and help the future work that will start from this issue.
A review would be highly appreciated!

[delvedor]

Hello! I've updated #970 with all the work done in the past weeks, please take a look!

[delvedor]

The new types has been shipped with 7.13, see the documentation to learn how to use them!

[ehaynes99]

The SearchRequest and SearchResponse types are not re-exported from api/new. Is this the intended way to import these types?

import { Client } from '@elastic/elasticsearch';
import type { Client as EsClient, ApiResponse } from '@elastic/elasticsearch/api/new';
import type { SearchRequest, SearchResponse } from '@elastic/elasticsearch/api/types';