Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored type definitions #1119

Merged
merged 42 commits into from Mar 23, 2020
Merged

Refactored type definitions #1119

merged 42 commits into from Mar 23, 2020

Conversation

delvedor
Copy link
Member

@delvedor delvedor commented Mar 16, 2020

This pr improves massively our type definitions, the best way to explain how, is to show it:

Old definitions (example taken from the old documentation):

import {
  Client,
  // Object that contains the type definitions of every API method
  RequestParams,
  // Interface of the generic API response
  ApiResponse,
} from '@elastic/elasticsearch'

const client = new Client({ node: 'http://localhost:9200' })

// Define the type of the body for the Search request
interface SearchBody {
  query: {
    match: { foo: string }
  }
}

// Complete definition of the Search response
interface ShardsResponse {
  total: number;
  successful: number;
  failed: number;
  skipped: number;
}

interface Explanation {
  value: number;
  description: string;
  details: Explanation[];
}

interface SearchResponse<T> {
  took: number;
  timed_out: boolean;
  _scroll_id?: string;
  _shards: ShardsResponse;
  hits: {
    total: number;
    max_score: number;
    hits: Array<{
      _index: string;
      _type: string;
      _id: string;
      _score: number;
      _source: T;
      _version?: number;
      _explanation?: Explanation;
      fields?: any;
      highlight?: any;
      inner_hits?: any;
      matched_queries?: string[];
      sort?: string[];
    }>;
  };
  aggregations?: any;
}

// Define the interface of the source object
interface Source {
  foo: string
}


// Define the search parameters
const searchParams: RequestParams.Search<SearchBody> = {
  index: 'test',
  body: {
    query: {
      match: { foo: 'bar' }
    }
  }
}

// Craft the final type definition
const response: ApiResponse<SearchResponse<Source>> = await client.search(searchParams)
console.log(response.body)

New definitions (example taken from the new test):

// Licensed to Elasticsearch B.V under one or more agreements.
// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information

import { expectType } from 'tsd'
import { Client, ApiResponse } from '../../'

const client = new Client({
  node: 'http://localhost:9200'
})

interface SearchBody {
  query: {
    match: { foo: string }
  }
}

interface ShardsResponse {
  total: number;
  successful: number;
  failed: number;
  skipped: number;
}

interface Explanation {
  value: number;
  description: string;
  details: Explanation[];
}

interface SearchResponse<T> {
  took: number;
  timed_out: boolean;
  _scroll_id?: string;
  _shards: ShardsResponse;
  hits: {
    total: number;
    max_score: number;
    hits: Array<{
      _index: string;
      _type: string;
      _id: string;
      _score: number;
      _source: T;
      _version?: number;
      _explanation?: Explanation;
      fields?: any;
      highlight?: any;
      inner_hits?: any;
      matched_queries?: string[];
      sort?: string[];
    }>;
  };
  aggregations?: any;
}

interface Source {
  foo: string
}

// No generics
{
  const response = await client.search({
    index: 'test',
    body: {
      query: {
        match: { foo: 'bar' }
      }
    }
  })

  expectType<any>(response.body)
  expectType<any>(response.meta.context)
}

// Define only the request body
{
  const response = await client.search<SearchBody>({
    index: 'test',
    body: {
      query: {
        match: { foo: 'bar' }
      }
    }
  })

  expectType<any>(response.body)
  expectType<any>(response.meta.context)
}

// Define request body and response body
{
  const response = await client.search<SearchBody, SearchResponse<Source>>({
    index: 'test',
    body: {
      query: {
        match: { foo: 'bar' }
      }
    }
  })

  expectType<SearchResponse<Source>>(response.body)
  expectType<any>(response.meta.context)
}

// Define request body, response body and the context
{
  const response = await client.search<SearchBody, SearchResponse<Source>, string>({
    index: 'test',
    body: {
      query: {
        match: { foo: 'bar' }
      }
    }
  })

  expectType<SearchResponse<Source>>(response.body)
  expectType<string>(response.meta.context)
}

TL;DR
Now every API makes better use of the generics and overloading, so you can (or not, by default request/response bodies are any) define the request/response bodies in the generics.

// request and response bodies are `any`
client.search(...)
// request body is `SearchBody` and response body is `any`
client.search<SearchBody>(...)
// request body is `SearchBody` and response body is `SearchResponse`
client.search<SearchBody, SearchResponse>(...)

This should not be a breaking change, as every generics defaults to any. It might happen to some users that the code breaks, but our test didn't detect any of it, probably because they were not robust enough. However, given the gigantic improvement in the developer experience, we have decided to release this change in the 7.x line.
This pr also introduces a vastly improved testing infrastructure for addressing the old fragile tests.
This pr should also help the future work we will do to address #933.

Don't get scared by the size of the pr, most of the code (in index.d.ts) is generated from a script, once you have reviewed a few lines, you have reviewed them all! ;)

Thank you to @fox1t for helping me designing this!

- Removed old test code
- Added tsd dev dependency
- Rewritten test with tsd
@villasv
Copy link
Contributor

villasv commented Mar 16, 2020

This is sooo good. Thank you so much for the hard work :-)

@joshdover joshdover self-requested a review March 16, 2020 19:07
Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

I tried to run node scripts/generate --tag=v7.6.1 but it didn't finish after 30 min. Are there any specific setup requirements to run the command locally?

scripts/utils/generateMain.js Show resolved Hide resolved
scripts/utils/generateMain.js Outdated Show resolved Hide resolved
return methods
} else {
let methods = [
{ key: `${api}<B = any, C = any> ()`, val: `Promise<ApiResponse<B, C>>` },
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference in output between this and https://github.com/elastic/elasticsearch-js/pull/1119/files#diff-a2dd684ce54a1e12240718626f065766R200 ? Probably we can merge them and always use { key: ${camelify(api)}<B = any, C = any> (), val: Promise<ApiResponse<B, C>> }, ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The client supports both snaked_cased apis and camelCased apis, for instance the latest async search will be available both as async_search and asyncSearch.
That if check only verifies if we should or not generate the camelCased api as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

just to confirm: not all snake_case API methods have camelCase counterparts?

Copy link
Member Author

Choose a reason for hiding this comment

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

All snake_case API methods have a camelCase counterpart :)

scripts/utils/generateMain.js Outdated Show resolved Hide resolved
scripts/utils/generateMain.js Outdated Show resolved Hide resolved
test/types/errors.test-d.ts Show resolved Hide resolved
@delvedor
Copy link
Member Author

I tried to run node scripts/generate --tag=v7.6.1 but it didn't finish after 30 min. Are there any specific setup requirements to run the command locally?

Nope, it should just work. Did it log something?
As first step (which usually is the one that takes longer), it will clone locally the elasticsearch repository, maybe something went wrong there?

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

I fear there is a bug and the very first run of the code generation should use master, I've just run in a freshly cloned repo

Now it works, great!
Thank you for starting this effort 🚢

- Use extends for defining the RequestBody generic to force the user
  following the same shape.
- BodyType and NDBodyType now accepts a generics to allow injecting
  more specific types in the future
@delvedor delvedor requested a review from mshustov March 20, 2020 15:05
@delvedor
Copy link
Member Author

I did another change, hopefully, the last one!
Before a method was defined as follows:

search<RequestBody = BodyType, Response = any, Context = unknown>(params: RequestParams.Search<RequestBody>, options: TransportRequestOptions): Promise<ApiResponse<Response, Context>>

Which worked well if the user was passing their own types, but if they were using the defaults, ts didn't "followed" the BodyType, because given it's a default value and it is used in the method signature, ts was overriding it with the user-provided value, making the following code valid:

 // note that number is not present in BodyType
client.search({ index: 'test', body: 42 })

For addressing this issue, now we are using extends in the RequestBody definition.

search<RequestBody extends BodyType, Response = any, Context = unknown>(params: RequestParams.Search<RequestBody>, options: TransportRequestOptions): Promise<ApiResponse<Response, Context>>

And as result the following code is no longer valid.

client.search({ index: 'test', body: 42 })

Furthermore, I've used RequestBody also as default value for Response. This might be a breaking change for the users, but they were checking in any case the returned type, now it is only more specific.

Finally, BodyType and NDBodyType accept a generic, which we will use in the future when we will start adding the request/response bodies definitions.
The code will look like this:

search<RequestBody extends BodyType<SearchBody>, Response = BodyType<SearchResponse>, Context = unknown>(params: RequestParams.Search<RequestBody>, options: TransportRequestOptions): Promise<ApiResponse<Response, Context>>

I fear we should accept the fact that we could release breaking changes for the type definitions in minors, as ts is a rapidly evolving language and they are releasing breaking changes in minors as well.

lib/Transport.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
- prefixed all generics with a T
- BodyType => RequestBody
- NDBodyType => RequestNDBody
- Added ResponseBody
@delvedor delvedor requested a review from Mpdreamz March 20, 2020 17:41
@delvedor delvedor merged commit 6c82a49 into master Mar 23, 2020
@delvedor delvedor deleted the improve-typescript-test branch March 23, 2020 10:38
@github-actions
Copy link
Contributor

The backport to 7.x failed:

The process 'git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-7.x 7.x
# Navigate to the new working tree
cd .worktrees/backport-7.x
# Create a new branch
git switch --create backport-1119-to-7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 6c82a4967ea5bed2370f9279226b59588b0a5950
# Push it to GitHub
git push --set-upstream origin backport-1119-to-7.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-7.x

Then, create a pull request where the base branch is 7.x and the compare/head branch is backport-1119-to-7.x.

delvedor added a commit that referenced this pull request Mar 23, 2020
* Updated types generation script

* Refactored api method definitions

* Updated test
- Removed old test code
- Added tsd dev dependency
- Rewritten test with tsd

* Removed unused dependencies

* Fixed definition

* Updated test

* Updated docs

* Improved events type definitions

* Updated test

* Minor fixes in the type definitons

* More type test

* Improved Transport type definitions

* Updated test

* Addressed comments

* Code generation

* Use RequestBody, Response and Context everywhere, also default Context to unknown

* Updated test

* body -> hasBody

* Fixed conflicts

* Updated code generation

* Improved request body type definition

* Updated code generation

* Use BodyType for both request and reponses generics
- Use extends for defining the RequestBody generic to force the user
  following the same shape.
- BodyType and NDBodyType now accepts a generics to allow injecting
  more specific types in the future

* API generation

* Updated test

* Updated docs

* Use BodyType also in ReponseError

* Removed useless client generics

* Renamed generics and types
- prefixed all generics with a T
- BodyType => RequestBody
- NDBodyType => RequestNDBody
- Added ResponseBody

* Updated test

* Updated docs

* Test ResponseBody as well

* Simplify overloads

* API generation

* Updated test

* Updated error types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants