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

The user should be able to change the requests timeout #716

Closed
bidoubiwa opened this issue Dec 10, 2020 · 11 comments · Fixed by #1554
Closed

The user should be able to change the requests timeout #716

bidoubiwa opened this issue Dec 10, 2020 · 11 comments · Fixed by #1554
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Dec 10, 2020

Since we removed axios and we are using fetch:

By default a fetch() request timeouts at the time setup by the browser. In Chrome, for example, this setting equals 300 seconds. That’s way longer than a user would expect for a simple network request to complete.

Screenshot 2020-12-10 at 17 31 41
Screenshot 2020-12-10 at 17 31 48

It lasted for about 2 min when I tried.

Should we give the possibility to users to define their own timeout when using MeiliSearch? This should then be done during client creation in the following way (or another):

const client = new window.MeiliSearch({
    host: 'http://google.com:81',
    apiKey: 'masterKey',
    timeout: 30000
  })

It is done this way in Algolia:

const index = client.initIndex('indexName');

const requestOptions = {
  // Set the readTimeout to 20 seconds
  timeouts: { read: 20 }
}

index.search('query string', requestOptions)
  .then(({ hits }) => {
    console.log(hits);
  });
@curquiza curquiza removed the SDK JS label Apr 14, 2021
@bidoubiwa bidoubiwa added the good first issue Good for newcomers label Sep 28, 2021
@bidoubiwa bidoubiwa changed the title Should the user be able to change the requests timeout The user should be able to change the requests timeout Sep 28, 2021
@ghost
Copy link

ghost commented Oct 3, 2021

I would like to work on this issue.

@bidoubiwa
Copy link
Contributor Author

Hey @AsianCat54x I assigned you to the PR :) Thanks for your interest in improving this project. We can only guarantee the assignment for 5 days. After which if someone else wants to give it a try we will change the assignation. Good luck with the PR!

@bidoubiwa bidoubiwa assigned ghost Oct 4, 2021
@ghost
Copy link

ghost commented Oct 4, 2021

Thanks! Can you just give me a few details on how to fix this issue. Even though I have just framed quite a few points that needs to be done, but I want to hear just a brief description. It would be very helpful.

@bidoubiwa
Copy link
Contributor Author

We should be able to provide the timeout when creating the client. A bit like the ruby client.

Then, pass the information to the request class. Where you can add the timeout to the fetch options if provided or use the default one if it is not.

If you need more information do not hesitate

@ghost
Copy link

ghost commented Oct 4, 2021

Oh I see! Thanks, I would start right away.

@bidoubiwa
Copy link
Contributor Author

bidoubiwa commented Oct 4, 2021

When creating a new client lets say

const client = new MeiliSearch({ host: 'http://...', apiKey: 'myKey' })

You provide an object that is considered the config object

class MeiliSearch {
  config: Config
  httpRequest: HttpRequests

  constructor(config: Config) {
    config.host = HttpRequests.addTrailingSlash(config.host)
    this.config = config
    this.httpRequest = new HttpRequests(config)
  }

As you can see the config object is passed to httpRequests, in which you can use it to play with the timeout

@bidoubiwa
Copy link
Contributor Author

Good luck!

@ghost
Copy link

ghost commented Oct 5, 2021

Sorry for a delay in solving this issue.

I have done this much so far:

(I think this is the right place to add the timeout to the fetch:

try {
      const response: Response = await fetch(constructURL.toString(), {
        ...config,
        method,
        body: JSON.stringify(body),
        headers: this.headers
}).then((res) => httpResponseErrorHandler(res))

)

So this there any other place to add timeout?

If not, then can you give me a case so that I can just test this out and check if it is working fine or not, Or directly open up a pull request.

(Since I am a new contributor, If the things I do act kinda childish, then I am really sorry. It would be my bad!)

@bidoubiwa
Copy link
Contributor Author

Hey @AsianCat54x
You are being very meticulous ❤️ Don't worry we love helping out new contributors. It is a hard step to create your first contribution and your first PR and we all had to go through it.

So, I would suggest you open a PR with your code suggestion. To create your PR our contributing shows you step by step how to do so. If some parts are obscure, feel free to ask more questions in this issue. Or join us on our slack (the link is in the above right corner of the page) and then come and talk to me (charlotte).

Once your PR is open I will give you suggestions to improve your code and add tests!

@ghost
Copy link

ghost commented Oct 5, 2021

Oh I see! Alright Im gonna create a pull request then ;)

@bidoubiwa
Copy link
Contributor Author

The issue is not fixed and waiting for contribution ❤️

ColinFrick added a commit to ColinFrick/meilisearch-js that referenced this issue Oct 4, 2022
ColinFrick added a commit to ColinFrick/meilisearch-js that referenced this issue Oct 12, 2022
@amit-ksh amit-ksh mentioned this issue Aug 3, 2023
3 tasks
@meili-bors meili-bors bot closed this as completed in 4af90a9 Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
3 participants