feat: add timeout property

[ColinFrick]

Pull Request

Related issue

Fixes #716

What does this PR do?

This PR adds the timeout property to the config. The AbortController is then used to abort the request if the timeout is reached. I have added a test to client.test.ts which cancels the timeout after 1 millisecond.

PR checklist

Please check if your PR fulfills the following requirements:

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

Thank you so much for contributing to Meilisearch!

[bidoubiwa]

Hey @ColinFrick Thanks again for your contribution :) about AbortController I have a few problems introducing it in the sources.

We are using abort-controller the npm package in the dev dependencies and not AbortController the standard API. The package is used only in the test filesearch.test.ts.

The reason why we do not use the standard lib is because, while it is a web API it has only been introduced to node in version 15 (as you can see if you click on history). Our library is still supporting node 14 for the moment until it becomes EOF.

Since meilisearch-js must work on both environments (web and node). By using the npm package, AbortController can be used in older versions of node ( < 15 ), so we need to keep using the npm package.

Now, the issue with the npm package is that it is not compatible with the latest typescript version. Maybe, we can just delay upgrading typescript until 30 Apr 2023 (date of EOF of node 14), but I'm wondering if the library is usable for a user using a more recent typescript version.

[ColinFrick]

As a workaround until node 14 support is dropped, controller?.signal could be cast to any:

      const response: any = await fetch(constructURL.toString(), {
        ...config,
        method,
        body: JSON.stringify(body),
        headers: this.headers,
        signal: controller?.signal as any,
      }).then((res) => httpResponseErrorHandler(res))

I ran yarn test with Typescript 4.8.4 on node 14 / 16 and it passed.

I just saw that my changes would actually break support for Abortable search. So I'll convert this PR to draft for the moment.

[bidoubiwa]

Hey @ColinFrick

Maybe a good way to know if this PR works in the new typescript environment is to update the version of typescript in this PR to their latest 4.8.4 version https://github.com/microsoft/TypeScript/releases/tag/v4.8.4.

If you are still working on this issue of course

[bidoubiwa]

This message is sent automatically

Thanks again for contributing to Meilisearch :heart: If you are participating in Hacktoberfest, and you would like to receive some gift from Meilisearch too, please complete this form.

[bidoubiwa]

Unfortunately it still fails on typescript 4.8.4 :(

src/http-requests.ts:118:9 - error TS2739: Type 'AbortSignal' is missing the following properties from type 'AbortSignal': reason, throwIfAborted

118         signal = controller.signal
            ~~~~~~

[ColinFrick]

Sorry about that. I'm currently swamped at work, but will check it out as soon as possible.

[bidoubiwa]

Don't worry 😊

[ColinFrick]

I was unable to get it to work with the current implementation of 'abort-controller'. I have extended the package with 'reason' and 'throwIfAborted' in this repo: https://github.com/ColinFrick/abort-controller

I have published the package on npm @colinfrick/abort-controller, but I don't know how feasible this solution is.

Maybe it really makes more sense to wait for EOL of node 14 in April.

[bidoubiwa]

Hey @ColinFrick

Wow, thanks a lot for your investigation on the subject. That's very interesting 🔥 It might be worth the wait though but it's absolutely very interesting what you did.

I was wondering how this PR would be compatible with this one: https://github.com/meilisearch/meilisearch-js/pull/1461

[ColinFrick]

Hey @bidoubiwa

It would need to be adapted because there could be a signal in fetchConfig.

Nonetheless maybe it makes more sense to just provide a timeout signal to the meilisearch client with { signal: AbortSignal.timeout(1000) }.

• Browser / Node >= 16.14.0: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout
• Node: https://www.npmjs.com/package/timeout-signal

That way there is less complexity inside the client, and less chance for conflicts / breaking.

[bidoubiwa]

Does this mean we should close this PR?

[ColinFrick]

I think so yes.

The only benefit of providing a timeout option would be that you can write { timeout: 1000 } instead of { signal: AbortSignal.timeout(1000) }. The introduced complexity is out of proportion with the minimal improvement in convenience.

[bidoubiwa]

Thanks anyway for this PR and for your contributions 🔥 They are always welcome and very valuable