Allow users to set custom timeout to the requests #1032

ghost · 2021-10-05T16:22:38Z

This pull request should fix #716

bidoubiwa

Hello! @AsianCat54x
Thanks for your contribution :)

Can I ask you the following before reviewing your code:

Change the title of the PR to something more explicit. We do not have any rules except that the name of the PR should follow the sentence This PR will.....

For example if my pr fixes a typo in the readme the title would be Fixes typo in README/

Fix the linting errors.
Both the code and the code formatting are tested. At this section of the CONTRIBUTING you can see that you have yarn style and yarn style:fix. The latter will fix the code automatically for you!

ghost · 2021-10-06T09:42:08Z

So shall I keep somethings like this: This PR will fix issue #716

Alright so I actually didnt see CONTRIBUTING.md. (My bad)

So all I need to do is lint the code with yarn.

So what about the AbortController error? (Here is a stackoverflow nswer - https://stackoverflow.com/a/58232108/16789038)

bidoubiwa · 2021-10-06T10:54:44Z

About the name while it does fix issue 716, the title should explain what the code in the PR fixes, not which issue is fixed. For that we use the Linked issue section.

So, the question you have to ask yourself is This PR will .... What does your code do?

About the AbortController issue it is because you don't import it. But 'ill get to your code review once the formatting and the name have been fixed.

ghost · 2021-10-06T12:05:13Z

Oh I see! Alright I will think of a good title

I dont know why I am getting error while running yarn style but I am trying to solve that!

bidoubiwa · 2021-10-06T12:06:17Z

did you try yarn style:fix ? It should automatically fix the majority of errors you have

ghost · 2021-10-06T12:12:15Z

Firstly when I ran yarn style, I got this error message ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin".. Searching in google gave this stackoverflow link

And btw, Is the title good?

Will using vscode's eslint do the trick?

bidoubiwa · 2021-10-06T12:17:40Z

The title of the issue is great!

Did you install the dependencies? yarn install ?

ghost · 2021-10-06T12:19:44Z

I ran yarn install but the error presisted, but now that I ran this command, the linter is working fine.

Now that the command is running, I am linting all the files and committing

bidoubiwa · 2021-10-06T13:06:05Z

Good work for the styling!

Now when you work on a PR it is better to get the tests running so you know when you are breaking the code.
In our case, to make the tests work you need a running instance of MeiliSearch on your computer.
As showed here you can run MeiliSearch with docker:

docker pull getmeili/meilisearch:latest # Fetch the latest version of MeiliSearch image from Docker Hub
docker run -p 7700:7700 getmeili/meilisearch:latest ./meilisearch --master-key=masterKey --no-analytics=true

Or with another method explained in the documentation.

You can also follow the getting started of MeiliSearch to understand better.

Tell me when you have a running MeiliSearch

bidoubiwa · 2021-10-06T13:23:53Z

Oke! Now you should run the tests by doing yarn test they should fail as your code is not working.

ghost · 2021-10-06T13:36:12Z

@bidoubiwa Good news here! All the test passed yay!

Here is a picture:

Note: I have edited some files

bidoubiwa · 2021-10-06T14:09:43Z

Are you using node version 16 ?

ghost · 2021-10-06T14:12:49Z

Yes, v16.10.0

bidoubiwa · 2021-10-06T15:35:34Z

I don't think it is working in previous versions of node. So the question is, do we want to add a package abort-controller to the package.

I don't think we want :s

ghost · 2021-10-06T16:05:10Z

Yeah, I installed abort controller and then ran the tests, it ran perfectly.

Should I commit the changes I have made?

If you dont want to add the abort-controller package, I could just use any other solutions to do the trick.

bidoubiwa · 2021-10-06T16:53:06Z

Are their other solutions than abort-controllers ? Yes I think tests fail with node 14 as abort controller is introduced in node 16.

bidoubiwa · 2021-10-09T12:41:30Z

Hey @AsianCat54x i will be reviewing your pr next week!

ghost · 2021-10-12T13:42:45Z

Hey @bidoubiwa, can you review?

ghost · 2021-10-18T11:24:22Z

@bidoubiwa, am I missing something?

bidoubiwa

Sorry very busy weeks
It seems to work fine :) Could you add some tests to ensure this is not provokings any bugs?

Also you used 'Error: Request Timed Out', but there is a MeiliSearchTimeoutError in /errors could you use this one when a timeout is thrown?

bidoubiwa · 2021-10-06T13:30:41Z

src/lib/http-requests.ts

+    const controller = new AbortController()
+
+    if (this.timeout === undefined) {
+      this.timeout = 3000
+    }
+
+    setTimeout(() => {
+      controller.abort()
+    }, this.timeout)
+


Could you move your code inside the try scope?

Actually, that error doesn't do anything. I kind of forget to remove that. Anyway, It will be required to add tests.

ghost · 2021-10-25T16:38:11Z

Hey, @bidoubiwa, I guess the only task left is to add tests, right? Its just that I am not able to create one. Can you help me by telling in which file I should make and where?

bidoubiwa · 2021-10-26T15:27:33Z

I would be glad to help you through this task. I'm adding the hacktoberfest-accepted to avoid being to late for the end of the event. If you are willing to wait a bit I will make an example of a test you can use.

If you are participating in Hacktoberfest, and you would like to receive a small gift from MeiliSearch too, please complete this form.

ghost · 2021-10-26T16:08:40Z

Yeah, I am participating in Hacktoberfest! Thanks, @bidoubiwa!

bidoubiwa · 2022-02-09T13:49:36Z

Hello @0xsapphir3
I wasn't able to make your code work. Unfortunately I do not have the time to make the research on the proper way to make it work.
If you find a working solution please consider re-opening a PR :)

ghost · 2022-02-11T09:12:06Z

Sure :)

puddingpy added 2 commits October 5, 2021 21:51

Fix issue #716

974c549

Setting default timeout if the timeout is undefined

8043c6c

bidoubiwa suggested changes Oct 6, 2021

View reviewed changes

ghost changed the title ~~Fix issue #716~~ Allow users to set custom timeout to the requests Oct 6, 2021

Lint the code

6dfef11

Remove the usage of AbortController

2d9bc22

ghost requested a review from bidoubiwa October 12, 2021 12:17

bidoubiwa suggested changes Oct 18, 2021

View reviewed changes

bidoubiwa added the hacktoberfest-accepted label Oct 26, 2021

meili-bot removed the hacktoberfest-accepted label Dec 6, 2021

bidoubiwa closed this Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to set custom timeout to the requests #1032

Allow users to set custom timeout to the requests #1032

ghost commented Oct 5, 2021 •

edited by ghost

Loading

bidoubiwa left a comment

ghost commented Oct 6, 2021 •

edited by ghost

Loading

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 •

edited by ghost

Loading

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 •

edited by ghost

Loading

bidoubiwa commented Oct 6, 2021

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 •

edited by ghost

Loading

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 •

edited by ghost

Loading

bidoubiwa commented Oct 6, 2021

bidoubiwa commented Oct 9, 2021

ghost commented Oct 12, 2021

ghost commented Oct 18, 2021

bidoubiwa left a comment

bidoubiwa Oct 6, 2021

ghost Oct 21, 2021

ghost commented Oct 25, 2021

bidoubiwa commented Oct 26, 2021

ghost commented Oct 26, 2021 •

edited by ghost

Loading

bidoubiwa commented Feb 9, 2022

ghost commented Feb 11, 2022

Allow users to set custom timeout to the requests #1032

Allow users to set custom timeout to the requests #1032

Conversation

ghost commented Oct 5, 2021 • edited by ghost Loading

bidoubiwa left a comment

Choose a reason for hiding this comment

ghost commented Oct 6, 2021 • edited by ghost Loading

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 • edited by ghost Loading

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 • edited by ghost Loading

bidoubiwa commented Oct 6, 2021

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 • edited by ghost Loading

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021

bidoubiwa commented Oct 6, 2021

ghost commented Oct 6, 2021 • edited by ghost Loading

bidoubiwa commented Oct 6, 2021

bidoubiwa commented Oct 9, 2021

ghost commented Oct 12, 2021

ghost commented Oct 18, 2021

bidoubiwa left a comment

Choose a reason for hiding this comment

bidoubiwa Oct 6, 2021

Choose a reason for hiding this comment

ghost Oct 21, 2021

Choose a reason for hiding this comment

ghost commented Oct 25, 2021

bidoubiwa commented Oct 26, 2021

ghost commented Oct 26, 2021 • edited by ghost Loading

bidoubiwa commented Feb 9, 2022

ghost commented Feb 11, 2022

ghost commented Oct 5, 2021 •

edited by ghost

Loading

ghost commented Oct 6, 2021 •

edited by ghost

Loading

ghost commented Oct 6, 2021 •

edited by ghost

Loading

ghost commented Oct 6, 2021 •

edited by ghost

Loading

ghost commented Oct 6, 2021 •

edited by ghost

Loading

ghost commented Oct 6, 2021 •

edited by ghost

Loading

ghost commented Oct 26, 2021 •

edited by ghost

Loading