Improve `HttpRequests` #1741

flevi29 · 2024-10-09T10:37:53Z

Pull Request

Sorry for the huge PR, but HttpRequests is core, and is used everywhere.

What does this PR do?

completely refactors http-requests.ts
- fixes Timeout implementation for requests should be re-thought #1654
- request parameters are now simple and straight forward
- types are reworked and no longer confusing
- makes full use of URL, Headers and URLSearchParams web standard features, which are safer and easier to work with
- makes use of Private properties to hide internal implementation that should not be public (this is widely supported and polyfillable)
fixes some EnqueuedTaskObjects not being converted to EnqueuedTask

Caution

BREAKING: From now on only some methods are public on HttpRequests (get, post, put, patch, delete), the rest of the properties and methods are internal implementation details, and are not meant to be directly accessed.

PR checklist

Please check if your PR fulfills the following requirements:

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

Thank you so much for contributing to Meilisearch!

codecov · 2024-10-09T10:40:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.44%. Comparing base (af27c78) to head (ed15092).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
+ Coverage   98.21%   98.44%   +0.23%     
==========================================
  Files          17       17              
  Lines        1565     1606      +41     
  Branches      333      343      +10     
==========================================
+ Hits         1537     1581      +44     
+ Misses         28       25       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flevi29 · 2024-10-10T17:22:57Z

src/clients/client.ts

+      body: params,
+    })) as EnqueuedTaskObject;
+
+    return new EnqueuedTask(taks);
  }


bug: previously this did not return EnqueuedTask, but rather EnqueuedTaskObject

flevi29 · 2024-10-10T17:29:26Z

tests/client.test.ts

+    test(`${permission} key: Create client with custom headers (object)`, async () => {
+      const key = await getKey(permission);
+      const client = new MeiliSearch({
+        ...config,
+        apiKey: key,
+        requestInit: {
+          headers: {
+            "Hello-There!": "General Kenobi",
+          },
        },
-      },
+      });
+
+      assert.isTrue(await client.isHealthy());
+
+      assert.isDefined(fetchSpy.mock.lastCall);
+      const [, requestInit] = fetchSpy.mock.lastCall!;
+
+      assert.isDefined(requestInit?.headers);
+      assert.instanceOf(requestInit!.headers, Headers);
+      assert.strictEqual(
+        (requestInit!.headers! as Headers).get("Hello-There!"),
+        "General Kenobi",
+      );
    });


Because headers are internal implementation details, to check them we have to spy on fetch now.

flevi29 · 2024-10-10T17:31:34Z

tests/unit.test.ts

 });

-test(`Client handles host URL with domain and path and no trailing slash`, () => {


Combine this test into the other.

flevi29 · 2024-10-10T17:34:48Z

src/indexes.ts

+  async update(data?: IndexOptions): Promise<EnqueuedTask> {
+    const task = (await this.httpRequest.patch({
+      relativeURL: `indexes/${this.uid}`,
+      body: data,
+    })) as EnqueuedTaskObject;

-    task.enqueuedAt = new Date(task.enqueuedAt);
-
-    return task;
+    return new EnqueuedTask(task);
  }


bug: previously this did not return EnqueuedTask

flevi29 · 2024-10-10T17:38:01Z

src/indexes.ts

  async deleteDocument(documentId: string | number): Promise<EnqueuedTask> {
-    const url = `indexes/${this.uid}/documents/${documentId}`;
-    const task = await this.httpRequest.delete<EnqueuedTask>(url);
-
-    task.enqueuedAt = new Date(task.enqueuedAt);
+    const task = (await this.httpRequest.delete({
+      relativeURL: `indexes/${this.uid}/documents/${documentId}`,
+    })) as EnqueuedTaskObject;

-    return task;
+    return new EnqueuedTask(task);
  }


bug: previously this did not return EnqueuedTask

flevi29 · 2024-10-10T17:38:20Z

src/indexes.ts

  async deleteAllDocuments(): Promise<EnqueuedTask> {
-    const url = `indexes/${this.uid}/documents`;
-    const task = await this.httpRequest.delete<EnqueuedTask>(url);
+    const task = (await this.httpRequest.delete({
+      relativeURL: `indexes/${this.uid}/documents`,
+    })) as EnqueuedTaskObject;

-    task.enqueuedAt = new Date(task.enqueuedAt);
-
-    return task;
+    return new EnqueuedTask(task);
  }


bug: previously this did not return EnqueuedTask

flevi29 · 2024-10-10T17:39:08Z

src/indexes.ts

  async updateSettings(settings: Settings): Promise<EnqueuedTask> {
-    const url = `indexes/${this.uid}/settings`;
-    const task = await this.httpRequest.patch(url, settings);
+    const task = (await this.httpRequest.patch({
+      relativeURL: `indexes/${this.uid}/settings`,
+      body: settings,
+    })) as EnqueuedTaskObject;

-    task.enqueued = new Date(task.enqueuedAt);
-
-    return task;
+    return new EnqueuedTask(task);
  }


bug: previously this did not return EnqueuedTask, I'm not going to mark the rest, there are more

src/indexes.ts

…-js into improve-http-request

Strift · 2025-01-08T12:06:58Z

src/http-requests.ts

+        for (const [key, val] of new Headers(extraHeaders).entries()) {
+          if (!headers.has(key)) {
+            headers.set(key, val);
+          }
+        }
+      }


Codecov CI says

Added lines #L184 - L189 were not covered by tests

Improve HttpRequests

9f41065

flevi29 added maintenance Issue about maintenance (CI, tests, refacto...) breaking-change The related changes are breaking for the users and removed maintenance Issue about maintenance (CI, tests, refacto...) labels Oct 9, 2024

flevi29 added 2 commits October 9, 2024 20:57

Convert props to private, refactor, adapt tests

a32ed2d

Fix type issue

118534d

flevi29 linked an issue Oct 10, 2024 that may be closed by this pull request

Timeout implementation for requests should be re-thought #1654

Open

flevi29 added 4 commits October 10, 2024 11:44

Improve timeout, refactor

942e889

Misc

455d8c3

Fix browser env test

d1b442b

Fix Node.js 18 fetch signal issue

c044319

flevi29 commented Oct 10, 2024

View reviewed changes

src/indexes.ts Outdated Show resolved Hide resolved

flevi29 added 4 commits October 11, 2024 10:49

Add extra RequestInit for search

e6236f6

Refactor

4a49790

Fix type issues

0e41c86

Fix some types

1949134

flevi29 marked this pull request as ready for review October 11, 2024 08:37

Make extraRequestInit function as it originally did

ace3412

flevi29 requested a review from brunoocasali October 11, 2024 11:00

flevi29 added 3 commits October 11, 2024 14:07

Remove unnecessary transformation

3854e13

Revert some type changes

d73d58d

Optimize

2d4849d

Merge branch 'main' into improve-http-request

8825781

Barabasbalazs mentioned this pull request Oct 21, 2024

TS eslint rules cleanup #1749

Draft

3 tasks

flevi29 added 7 commits October 30, 2024 11:06

Merge with main branch

db9aa94

Merge branch 'main' into improve-http-request

432f338

Merge branch 'main' into improve-http-request

d70f962

Merge with main

aa10d8c

Merge branch 'improve-http-request' of github.com:flevi29/meilisearch…

f81e7ca

…-js into improve-http-request

Merge branch 'main' into improve-http-request

f231e95

Merge with main

8f29bc2

flevi29 added the bug Something isn't working label Dec 24, 2024

flevi29 added 4 commits December 27, 2024 13:59

Merge branch 'main' into improve-http-request

edff9a9

Fix types

36c174f

Merge branch 'main' into improve-http-request

a26cc1d

Merge branch 'main' into improve-http-request

f0998be

Strift added the maintenance Issue about maintenance (CI, tests, refacto...) label Jan 8, 2025

Strift reviewed Jan 8, 2025

View reviewed changes

flevi29 added 3 commits January 8, 2025 14:15

Merge with main

f7b1e88

Fix type and style issues

a32c773

Test extra headers as well

aaaaf3b

flevi29 requested a review from Strift January 8, 2025 13:35

Merge branch 'main' into improve-http-request

ed15092

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `HttpRequests` #1741

Improve `HttpRequests` #1741

flevi29 commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

flevi29 Oct 10, 2024

flevi29 Oct 10, 2024 •

edited

Loading

flevi29 Oct 10, 2024

flevi29 Oct 10, 2024

flevi29 Oct 10, 2024

flevi29 Oct 10, 2024

flevi29 Oct 10, 2024

Strift Jan 8, 2025

flevi29 Jan 8, 2025

		});

		test(`Client handles host URL with domain and path and no trailing slash`, () => {

Improve HttpRequests #1741

Are you sure you want to change the base?

Improve HttpRequests #1741

Conversation

flevi29 commented Oct 9, 2024 • edited Loading

Pull Request

What does this PR do?

PR checklist

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

flevi29 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve `HttpRequests` #1741

Improve `HttpRequests` #1741

flevi29 commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

flevi29 Oct 10, 2024 •

edited

Loading