Skip to content

Switched request and response generics position #1132

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

Merged
merged 14 commits into from
Apr 6, 2020

Conversation

delvedor
Copy link
Member

As titled, closes #1130.

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 do agree the current update brings more value and inlined with typical network client interfaces (axios)
I concern about the update process. Would it be hard to release a major release to follow the proper semver?

}

async function test () {
const scrollSearch = client.helpers.scrollSearch<Source, SearchResponse<Source>>({
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it follow the same type order as other generics? <ResponseBody, RequestBody, Context, Document>?

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 reason why I put TDocument as the first parameter, is because the scrollSearch returns AsyncIterable<ScrollSearchResponse<TDocument, TResponse, TContext>>, and ScrollSearchResponse is defined as follows:

export interface ScrollSearchResponse<TDocument = unknown, TResponse = Record<string, any>, TContext = unknown> extends ApiResponse<TResponse, TContext> {
  clear: () => Promise<void>
  documents: TDocument[]
}

Having TDocument as first parameter would allow users to access directly response.documents instead of accessing the full search response.

@delvedor
Copy link
Member Author

delvedor commented Apr 3, 2020

I do agree the current update brings more value and inlined with typical network client interfaces (axios)

👍

I concern about the update process. Would it be hard to release a major release to follow the proper semver?

I can't release a major until Elasticsearch 8, as the client strictly follows the Elastic Stack release train.
I'm fine doing a breaking change specifically for the typescript definitions for two reasons:

  1. It will improve massively the developer experience.
  2. The TypeScript core team does release breaking changes in minor releases, so I'm assuming the community is ok with this behavior.

@delvedor delvedor merged commit 6779f3b into master Apr 6, 2020
@delvedor delvedor deleted the switch-req-res-generics branch April 6, 2020 10:45
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2020

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-1132-to-7.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 6779f3b11abdfda53848e5fa9d63194b737adafb
# Push it to GitHub
git push --set-upstream origin backport-1132-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-1132-to-7.x.

delvedor added a commit that referenced this pull request Apr 6, 2020
* Updated code generation

* Switched request and response generics position

* Updated test

* API generation

* Removed unused generics

* Test type definitions for callback style API as well

* Fix comments

* Fix conflict

* API generation

* Updated type def
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.

Switch Request and Response generics
2 participants