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

Post search route #529

Merged
merged 12 commits into from
Aug 3, 2020
Merged

Post search route #529

merged 12 commits into from
Aug 3, 2020

Conversation

bidoubiwa
Copy link
Contributor

@bidoubiwa bidoubiwa commented Jul 27, 2020

This PR creates an additional parameter method on the search route. This lets the user decide if he wants to use the POST method (faster) or the GET method.

The tests in the CI will fail until MeiliSearch v0.13.0 is out.

Currently, tested with the v0.13.0rc0, tests are failing because of:

Typescript issues

1 Itterating through an object with Typescrypt

The typings in the following code sample seems to be more complicated than it should be

function removeUndefinedFromObject(object: Record<string, any>): object {
  return Object.keys(object).reduce((acc: Record<string, any>, key: string) => {
    if (object[key] !== undefined) {
      acc[key] = object[key]
    }
    return acc
  }, {})
}

2 Using fixed values as types in jest describe

Here is the type of my function

  search: <P extends SearchParams<T>>(
    query: string,
    options?: P,
    method?: 'POST' | 'GET' // method can only be `POST` or `GET`
  ) => Promise<SearchResponse<T, P>>

Here is how I do multiple tests on different methods:

describe.each([
  { client: masterClient, permission: 'Master' },
  { client: privateClient, permission: 'Private' },
  { client: publicClient, permission: 'Public' },
])('Test on search', ({ client, permission }) => {
  describe.each([
    { method: 'POST', permission, client },
    { method: 'GET', permission, client },
  ])('Test on search', ({ client, permission, method }) => {
    beforeAll(async () => {
      ...
    })
    test(`${permission} key: Basic ${method} search`, async () => {
      if (method === 'POST' || method == 'GET') {
        await client
          .getIndex(index.uid)
          .search('prince', {}, method)
          .then((response) => {
            expect(response).toHaveProperty('hits', expect.any(Array))
            ...
          })
      }
    })
})

As you can see I need to add the if condition because otherwise I recieve this error:

Argument of type 'string' is not assignable to parameter of type '"POST" | "GET"'.

I tried a lot of ways to add the types in the describe function but I couln't find how.
@emyann maybe you have an idea ?

@bidoubiwa bidoubiwa changed the base branch from master to bump-meilisearch-v0.13.0 July 27, 2020 17:56
@bidoubiwa bidoubiwa changed the base branch from bump-meilisearch-v0.13.0 to master July 27, 2020 18:00
@bidoubiwa bidoubiwa changed the base branch from master to bump-meilisearch-v0.13.0 July 28, 2020 09:01
@bidoubiwa bidoubiwa requested a review from curquiza July 28, 2020 09:16
@curquiza curquiza added the breaking-change The related changes are breaking for the users label Jul 28, 2020
This was referenced Jul 28, 2020
@curquiza
Copy link
Member

@bidoubiwa could you rebase your branch please? 🙂

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Just a suggestion, not sure about that: could we have the same tests running for the POST and then with the GET? The goal is to avoid tests duplication.

@emyann
Copy link
Contributor

emyann commented Jul 29, 2020

@bidoubiwa This is a Type widening behaviour in Typescript. One thing you could try is to use a const type assertion:

describe.each([
  { method: 'POST' as const,  },
  { method: 'GET' as const,  },
])('Test on search', ({  method }) => {...})

Then the method property instead of being a widened string, will be of type POST | GET
Screen Shot 2020-07-29 at 12 48 23 PM

Does it answer your question ? :)

@bidoubiwa
Copy link
Contributor Author

@emyann Yes ! This worked great :) Thanks a lot for answering

Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Question:

  • What is the difference between tests/search_tests.ts and tests/typed_search_tests.ts?

Plus, could you update the search method prototype in the README? 🙂

Comment on lines +124 to +127
} else {
throw new MeiliSearchError(
'method parameter should be either POST or GET'
)
Copy link
Member

Choose a reason for hiding this comment

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

Cannot Typscript handle this part? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes, but not if the user is not using typescript but javascript. In which case I prefer the error to be really obvious.

@@ -11,7 +11,7 @@
"declarationDir": "./dist/types",
"esModuleInterop": true,
"allowSyntheticDefaultImports": true,
"lib": ["ES2015"]
"lib": ["ES2015", "ESNext"]
Copy link
Member

Choose a reason for hiding this comment

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

What the purpose of ESNext? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This are the rules to transpile typescript to javascript. By saying ESNext I inform the transpiler that I will be writing with the last features of javascript. This will make it possible for me to write for example optionnal chaining. These features are not released in an official Ecmascript version but are bound to be in the next one.

if (response.data?.hits?.something)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanations! 😁

@bidoubiwa
Copy link
Contributor Author

What is the difference between tests/search_tests.ts and tests/typed_search_tests.ts?

In type_search_search every time I use an index I give the type of the document that will be inside that index:

 await client
        .getIndex<Movie>(index.uid)

Which means that the compiler should raise an error if I try to access a field that does not exist in movie.

interface Movie {
  id: number
  title: string
  comment?: string
  genre?: string
}
response.hits[0].director // ERROR because director is not defined in Movie interface

In search_tests I have not added <Movie>. When not adding it, typescript infer that the document could have anything inside it. Thus is way less strict on what I do.

response.hits[0].director // No errors but undefined

@bidoubiwa bidoubiwa merged commit bef130d into bump-meilisearch-v0.13.0 Aug 3, 2020
@bidoubiwa bidoubiwa deleted the post_search_route branch August 3, 2020 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants