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

types: fix search default type of search and searchGet #1416

Merged
merged 7 commits into from
Dec 12, 2022

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Dec 11, 2022

Pull Request

Related issue

No

What does this PR do?

This pr fix the default genetic type params of search and searchGet, it should use generic type param of index

expected type behavior in readme

client.index<T>('xxx').search(...): Promise<SearchResponse<T>>

current actually type, because search has a default generic type param Record<string, any>:

client.index<T>('xxx').search(...):

is implicit

client.index<T>('xxx').search<Record<string, any>>(...):

and it's actually

client.index<T>('xxx').search(...): Promise<SearchResponse<Record<string, any>>>

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@trim21
Copy link
Contributor Author

trim21 commented Dec 11, 2022

and what's the point of Document?

export type Document<T = Record<string, any>> = T
export type Documents<T = Record<string, any>> = Array<Document<T>>

Document<T> is just T, should it be export type Document<T extends Record<string, any>> = T?
Documents<T> is just Array<T>, should it also be export type Documents<T extends Record<string, any>> = Array<Document<T>> ?

@trim21 trim21 changed the title types: fix search default type of search and searchRaw types: fix search default type of search and searchGet Dec 11, 2022
@bidoubiwa bidoubiwa added skip-changelog The PR will not appear in the release changelogs maintenance Issue about maintenance (CI, tests, refacto...) labels Dec 12, 2022
@bidoubiwa bidoubiwa self-requested a review December 12, 2022 11:03
Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

In typed_search.test.ts

Could you also add the following? That's what is failing the tests

export interface Movie {
  id: number
  title: string
  genre?: string
  comment?: string
  isNull?: null
  isTrue?: true
}

interface NestedDocument {
  id: number
  title: string
  info: {
    comment?: string
    reviewNb?: number
  }
}

const dataset: Movie[] = [

I know this is not the best design 😅 We'll work on that later

tests/search.test.ts Outdated Show resolved Hide resolved
tests/env/typescript-node/src/index.ts Outdated Show resolved Hide resolved
@trim21
Copy link
Contributor Author

trim21 commented Dec 12, 2022

I fixed this error by add isNull in both Movie object.

do you perfer @ts-ignore or @ts-expect-error here?

@trim21
Copy link
Contributor Author

trim21 commented Dec 12, 2022

it's still '1 change requested', any thing I need to do?

@bidoubiwa
Copy link
Contributor

Document is just T, should it be export type Document<T extends Record<string, any>> = T?
Documents is just Array, should it also be export type Documents<T extends Record<string, any>> = Array<Document> ?

You'r right, Document is just T. I made a Type mostly for readability to avoid writing a long type in the method prototype.

With the same logic as with getSearch and search, I think it should be D = T.

What do you think?

@trim21
Copy link
Contributor Author

trim21 commented Dec 12, 2022

if Document<T> is just T then I think we should just remove it.

@bidoubiwa
Copy link
Contributor

Yep, should be removed

@trim21
Copy link
Contributor Author

trim21 commented Dec 12, 2022

so is there anything blocking this PR now?

@bidoubiwa
Copy link
Contributor

Oh I thought you were going to remove Document in this PR as well. My bad :) I'll open an issue for that

Copy link
Contributor

@bidoubiwa bidoubiwa left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@bidoubiwa bidoubiwa added enhancement New feature or request and removed skip-changelog The PR will not appear in the release changelogs labels Dec 12, 2022
@bidoubiwa
Copy link
Contributor

bors merge

@trim21
Copy link
Contributor Author

trim21 commented Dec 12, 2022

Oh I thought you were going to remove Document in this PR as well. My bad :) I'll open an issue for that

Removing it is not very related to this pr I this, should be separated to another pr…

@meili-bors
Copy link
Contributor

meili-bors bot commented Dec 12, 2022

@meili-bors meili-bors bot merged commit 1287b3e into meilisearch:main Dec 12, 2022
@bidoubiwa
Copy link
Contributor

#1421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maintenance Issue about maintenance (CI, tests, refacto...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants