Removing Undici Dependency #363
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR removes the undici HTTP client in favor of the built-in Fetch API (Request/Response) and updates tests and package settings accordingly.
- Migrate
HttpRequestandHttpResponseto use globalRequest/ResponseandHeaders - Remove undici imports and dependency, bump package version to 5.0.0 and Node engine to >=20.0
- Adjust tests to drop undici types and use
asassertions
Reviewed Changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/http/extractHttpUserFromHeaders.test.ts | Remove unused Headers import from undici |
| test/http/HttpRequest.test.ts | Drop undici File import, switch to as File, loosen rejection check |
| test/converters/toRpcHttp.test.ts | Remove unused Headers import from undici |
| src/http/extractHttpUserFromHeaders.ts | Remove unused undici Headers import |
| src/http/HttpResponse.ts | Replace undici Response/Headers with global Fetch API, adjust methods |
| src/http/HttpRequest.ts | Replace undici Request with global Fetch API, adjust methods |
| package.json | Bump version to 5.0.0, Node >=20.0, remove undici dependency |
Comments suppressed due to low confidence (2)
test/http/HttpRequest.test.ts:137
- [nitpick] The test no longer asserts a specific error message or type. Consider matching the exact error text or class to ensure the correct failure reason is being tested.
await expect(req.formData()).to.eventually.be.rejectedWith();
test/http/HttpRequest.test.ts:96
- The test uses the
Filetype without importing it; after removingimport { File } from 'undici', you need to importFilefrom a valid source or adjust the test to work with the available global API.
const file = parsedForm.get('myfile') as File;
| get body(): ReadableStream<Uint8Array> | null { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return this.#res.body as any; // Type compatibility between global and Node.js ReadableStream | ||
| } |
There was a problem hiding this comment.
[nitpick] Avoid using any and disabling lint rules. Consider defining a proper type alias or wrapper for the stream to maintain type safety.
| get body(): ReadableStream<Uint8Array> | null { | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | |
| return this.#res.body as any; // Type compatibility between global and Node.js ReadableStream | |
| } | |
| get body(): ReadableStream<Uint8Array> | null { | |
| return this.#res.body as ReadableStream<Uint8Array> | null; // Type compatibility between global and Node.js ReadableStream | |
| } |
Copilot uses AI. Check for mistakes.
| return this.#uReq.body; | ||
| get body(): ReadableStream<Uint8Array> | null { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return this.#req.body as any; // Type compatibility between global and Node.js ReadableStream |
There was a problem hiding this comment.
[nitpick] Using any here bypasses type checks; consider creating a proper type guard or wrapper for the request body stream to keep type safety.
Copilot uses AI. Check for mistakes.
| const req = new Request(url, { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| body: body as any, // Node.js Readable stream compatibility | ||
| duplex: 'half', | ||
| method: nonNullProp(proxyReq, 'method'), | ||
| headers, | ||
| }); | ||
| } as any); // Global Request constructor compatibility |
There was a problem hiding this comment.
[nitpick] Casting the init object to any can hide real type errors. You could define or extend a RequestInit type to properly handle the duplex and stream compatibility instead of using any.
Copilot uses AI. Check for mistakes.
No description provided.