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

Add ability to abort search request #636

Merged
merged 2 commits into from
Oct 21, 2020
Merged

Conversation

neupauer
Copy link
Contributor

This PR adds the ability to insert additional config into the search request, e.g., signal to abort the request.

Resolve #630

@bidoubiwa
Copy link
Contributor

Hey @neupauer, thanks so much for these great contributions. A few questions.

I see that you changed the config type with Partial<Request>.

  • How does this affect a typeScript user ?
  • Why did you had to change the type?

@neupauer
Copy link
Contributor Author

Hi @bidoubiwa, thanks for the feedback.

The reason for the Partial<Request> is that we do not want to provide the whole interface (Request), but only part of it.

Example:

client.getIndex("movies").search("query", {}, method, {
  // only part of Request interface, e.g., signal
  signal: controller.signal,
})

Example without Partial<T>:

client.getIndex("movies").search("query", {}, method, {
  // the whole interface is required
  cache,
  credentials,
  destination,
  headers,
  signal,
  //...
})

Partial<Request> is basically the same as the Request, but all properties are optional.

More information can be found at: Typescript | Utility Types

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.

Thanks a lot for your great answers!
Could you add a test that does two searches in the following scenario

  • make a search A
  • make a search B
  • abord A
  • check if B is successful and A has been aborted

@neupauer
Copy link
Contributor Author

@bidoubiwa Yes, sure, I updated the tests and also add some notes to the README.md

Note:
I added 4 concurent search requests (3 with signal one without) and abort only one

bidoubiwa
bidoubiwa previously approved these changes Oct 19, 2020
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 🔥 Just asking a second eye on this to be sure @curquiza

@bidoubiwa bidoubiwa requested a review from curquiza October 19, 2020 18:26
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.

A small change in the README, otherwise, it's perfect 😁 Thanks for this really clear PR!

README.md Outdated Show resolved Hide resolved
Signed-off-by: Peter Neupauer <[email protected]>
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.

Awesome! Thanks again!!

@bidoubiwa
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Oct 21, 2020

Build succeeded:

@bors bors bot merged commit 7cc2b03 into meilisearch:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User should be able to drop a pending search request - Discussion
3 participants