Improve HttpRequests
#1741
Improve HttpRequests
#1741
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1741 +/- ##
==========================================
+ Coverage 98.21% 98.61% +0.40%
==========================================
Files 17 17
Lines 1565 1594 +29
Branches 333 344 +11
==========================================
+ Hits 1537 1572 +35
+ Misses 28 22 -6 ☔ View full report in Codecov by Sentry. |
flevi29
added
maintenance
src/clients/client.ts
Outdated
| body: params, | ||
| })) as EnqueuedTaskObject; | ||
|
|
||
| return new EnqueuedTask(taks); | ||
| } |
There was a problem hiding this comment.
bug: previously this did not return EnqueuedTask, but rather EnqueuedTaskObject
tests/client.test.ts
Outdated
| 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", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Because headers are internal implementation details, to check them we have to spy on fetch now.
There was a problem hiding this comment.
Hi @flevi29, thanks for the great work 🙌
I would love to merge this too before the new release!I have a few questions:
Generic methods types
Any reason to switch from method<Type>() to method() as Type?
One example is in indexes.ts: await this.httpRequest.get(...) as IndexObject;, but there are other occurrences of this.
The former approach seems to provide a better DX.
Renaming config to extraRequestInit
I'm all for renaming it in variables and types used internally to make the code base more self-explanatory. But I'm not sure about changing the user-facing field names.
I feel like config / requestConfig are more familiar field names for configuring the underlying HTTP client/request.
Plus, changing them would be a breaking change, right?
tests/search.test.ts
Outdated
| expect(e.name).toEqual("MeiliSearchRequestError"); | ||
| } | ||
| }); | ||
|
|
||
| test(`${permission} key: search should be aborted on already abort signal`, async () => { |
There was a problem hiding this comment.
I don't understand the name of this test. Can you explain, or rephrase it, please?
There was a problem hiding this comment.
First it tests whether search gets immediately aborted on an already aborted signal, then it test whether it gets aborted after the request has already been sent. I have renamed it to ${permission} key: search should be aborted on abort signal.
The idea was that when we are calling an external web API, the shape of the response cannot be guaranteed, so we're casting to be explicit about this fact. But you are right, it is an overkill, I'm moving the casting part to
I felt like "RequestInit" is more explicit, because that's literally what it's named and what we're using and allowing via types and it is the standard at this point, while It is named "extra", because we're applying 3 layers of Also there's an open issue about adding this extra config to all of the methods #1476 |
|
For now I've made the requested changes, but I do plan to add better documentation to |
|
I'm also deprecating |
It's not a breaking change, because it's a positional parameter, not an object property in the |
|
Actually the private properties change is non-breaking, because |
|
Hey @flevi29, sorry we couldn't get this in time for #1826. The next pre-release cycle starts on January 27 and I wanted to have time at least 2 weeks to fix bugs before getting into the new features implementation. But I think this PR is already in a good state. We'll probably be able to merge it soon after updating the package for Meilisearch 1.13. |
|
Alright, no problem, I just didn't know you guys talked before releasing. But to be fair I'm also excited to get my PRs merged 😅. |
|
Once v1.13 is out, merging your PR will be the priority @flevi29 😉 |
Pull Request
Sorry for the huge PR, but
HttpRequestsis core, and is used everywhere.What does this PR do?
EnqueuedTaskObjects not being converted toEnqueuedTaskCaution
BREAKING
Rename
Config.requestConfig->Config.requestInit,signalcan no longer be passed to it.Warning
Deprecate
Config.httpClient#1824PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!