feat: impl fetch #542
feat: impl fetch #542
Conversation
- keep urllib:request, urllib:response channel message - add urllib:fetch:request, urllib:fetch:response channel message
WalkthroughThe changes introduce several new functionalities and modifications across multiple files in the codebase. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #542 +/- ##
==========================================
- Coverage 97.47% 94.70% -2.78%
==========================================
Files 7 9 +2
Lines 990 1208 +218
Branches 260 281 +21
==========================================
+ Hits 965 1144 +179
- Misses 21 60 +39
Partials 4 4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (7)
test/fetch.test.ts (2)
48-51: LGTM: Fetch-specific assertions are comprehensive.The assertions for fetch-specific diagnostics messages are well-implemented, covering fetch, response, and timing information.
Consider adding an assertion to check if the
timingInfocontains expected properties, such asstartTimeorendTime, to ensure the timing data is meaningful.assert(fetchResponseDiagnosticsMessage!.timingInfo.startTime); assert(fetchResponseDiagnosticsMessage!.timingInfo.endTime);
1-57: Overall, excellent test implementation with room for minor enhancements.This test file provides a comprehensive check of the fetch functionality, including diagnostics messages and dispatcher pool stats. The structure is solid and follows testing best practices.
To further improve the test:
- Consider adding more specific test cases to cover different scenarios (e.g., error cases, different response types).
- Add comments to explain the purpose of certain steps, especially for the dispatcher pool stats.
- Consider parameterizing the test to run with different configurations of the FetchFactory client options.
Great job on implementing this test suite!
src/HttpAgent.ts (1)
Line range hint
39-66: Well-structured implementation of lookupFunction!The new implementation of
lookupFunctionis well-designed and incorporates the necessary logic for DNS lookup and address checking. Great job on handling both string and array address formats for compatibility.A minor suggestion for improved readability:
Consider extracting the address checking logic into a separate method. This would make the
lookupFunctionmore concise and easier to understand at a glance. For example:private checkAddressValidity(address: string | { address: string, family: number }[], family: number, hostname: string): Error | null { if (typeof address === 'string') { if (!this.options.checkAddress(address, family, hostname)) { return new IllegalAddressError(hostname, address, family); } } else if (Array.isArray(address)) { for (const addr of address) { if (!this.options.checkAddress(addr.address, addr.family, hostname)) { return new IllegalAddressError(hostname, addr.address, addr.family); } } } return null; }Then, you can use this method in the
lookupFunction:const error = this.checkAddressValidity(address, family, hostname); if (error) return (callback as any)(error, address, family);This refactoring would improve the overall readability and maintainability of the code.
src/Request.ts (3)
164-164: LGTM! Consider adding JSDoc comment forctx.The addition of the optional
ctxproperty toRequestMetais a good improvement for flexibility. The use of theunknowntype is appropriate for a generic context.Consider adding a JSDoc comment to explain the purpose and potential usage of the
ctxproperty. For example:/** Optional context information for the request */ ctx?: unknown;
166-169: LGTM! Consider adding JSDoc comments and type imports.The addition of the
FetchMetatype aligns well with the PR objectives for implementing new fetch-related channel messages. The structure is consistent with existing types.Consider the following improvements:
- Add JSDoc comments to explain the purpose of the
FetchMetatype and its properties.- Import the
Requesttype explicitly for better code readability.Example implementation:
import type { Request } from 'undici'; /** Metadata for fetch requests */ export type FetchMeta = { /** Unique identifier for the fetch request */ requestId: number; /** The fetch request object */ request: Request; };
6-6: Overall, the changes look good and align with the PR objectives.The addition of the
FetchMetatype and the modification ofRequestMetaenhance the functionality for handling fetch requests. The changes are consistent with the existing codebase structure and naming conventions.As the project evolves to include more fetch-related functionality, consider creating a separate file for fetch-specific types if the number of such types increases. This would help maintain a clear separation of concerns and improve code organization.
Also applies to: 165-169
src/HttpClient.ts (1)
69-69: Clarify the Commented-out Property 'ALPNNegotiatedProtocol'The property
ALPNNegotiatedProtocolis commented out within theUnidiciTimingInfointerface. If this property is intended to be part of the interface, consider uncommenting it or providing a comment explaining its current status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- src/FetchOpaqueInterceptor.ts (1 hunks)
- src/HttpAgent.ts (3 hunks)
- src/HttpClient.ts (5 hunks)
- src/Request.ts (2 hunks)
- src/fetch.ts (1 hunks)
- src/index.ts (2 hunks)
- src/utils.ts (2 hunks)
- test/fetch.test.ts (1 hunks)
🧰 Additional context used
🪛 Biome
src/fetch.ts
[error] 59-262: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 GitHub Check: codecov/patch
src/fetch.ts
[warning] 68-69: src/fetch.ts#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 84-91: src/fetch.ts#L84-L91
Added lines #L84 - L91 were not covered by tests
[warning] 93-98: src/fetch.ts#L93-L98
Added lines #L93 - L98 were not covered by tests
[warning] 101-106: src/fetch.ts#L101-L106
Added lines #L101 - L106 were not covered by tests
[warning] 117-118: src/fetch.ts#L117-L118
Added lines #L117 - L118 were not covered by tests
[warning] 227-237: src/fetch.ts#L227-L237
Added lines #L227 - L237 were not covered by testssrc/utils.ts
[warning] 198-201: src/utils.ts#L198-L201
Added lines #L198 - L201 were not covered by tests
🔇 Additional comments (13)
test/fetch.test.ts (4)
1-20: LGTM: Imports and test setup are well-structured.The import statements and test suite setup are appropriate and follow best practices. The use of
beforeAllandafterAllhooks ensures proper setup and teardown of the test server.
41-47: LGTM: Fetch execution and basic assertions are correct.The fetch execution and initial assertions are well-implemented. They correctly check for the presence of the response and the expected diagnostics messages.
22-39: LGTM: Comprehensive test case setup with a minor clarification needed.The test case setup is well-structured, covering all relevant diagnostics channels. The use of type assertions for diagnostics messages ensures type safety.
However, could you clarify the purpose of setting up FetchFactory client options with an empty object? Is this intentional for testing default behavior?
✅ Verification successful
LGTM: Comprehensive test case setup with no further issues found.
The test case setup is well-structured, covering all relevant diagnostics channels. The use of type assertions for diagnostics messages ensures type safety.
The use of an empty object for setting FetchFactory client options is confirmed to be intentional for testing default behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if there are any other occurrences of setClientOptions with non-empty objects rg --type typescript 'FetchFactory\.setClientOptions\([^{}]' -g '!test/fetch.test.ts'Length of output: 122
Script:
#!/bin/bash # Verify if there are any other occurrences of setClientOptions with non-empty objects in TypeScript files rg 'FetchFactory\.setClientOptions\([^{}]' -g '*.ts' -g '!test/fetch.test.ts'Length of output: 79
53-55: LGTM: Dispatcher pool stats assertions are present, but could use more context.The retrieval and assertion of dispatcher pool stats are implemented correctly. However, it would be beneficial to add a comment explaining the purpose of these stats and what specific properties we expect to see in a typical scenario.
Consider adding assertions for specific expected properties in the stats object to make the test more robust.
Add a comment explaining the purpose of dispatcher pool stats and their significance in this test.
src/HttpAgent.ts (3)
11-16: Excellent improvement to HttpAgentOptions!The change from a type alias to an interface extending
Agent.Optionsis a great improvement. This modification:
- Enhances type checking capabilities.
- Allows for easier future extensions to the options.
- Maintains compatibility with existing
Agent.Options.The new properties (
lookup,checkAddress,connect, andallowH2) align well with the PR objectives of implementing new fetch-related capabilities.
67-68: Effective use of spread operator in super call!The updated super call effectively incorporates the new options structure:
- Using
...baseOptsensures all base options are passed to the superclass.- The
connectobject correctly includes the newlookupFunctionandallowH2option.This change aligns well with the new options structure and functionality introduced in this PR.
Line range hint
1-92: Overall excellent implementation of fetch-related enhancements!The changes in this file successfully implement the new fetch-related capabilities mentioned in the PR objectives. Key improvements include:
- Enhanced
HttpAgentOptionsstructure for better type checking and extensibility.- Well-implemented
lookupFunctionthat incorporates DNS lookup and address checking.- Effective use of new options in the
HttpAgentconstructor.The code quality is high, with only minor suggestions for improvement in terms of readability. These changes will significantly enhance the urllib module's capabilities for handling fetch operations.
src/utils.ts (1)
5-7: Imports added are appropriate and necessaryThe additions of
SocketInfo,symbols, andIncomingHttpHeadersimports are appropriate for the new functionalities introduced in the module.src/HttpClient.ts (5)
40-40: Importing 'updateSocketInfo' FunctionThe addition of
updateSocketInfoto the import statement ensures that the function is available for use within this file. This change is appropriate.
50-50: Exporting 'PROTO_RE' ConstantExporting
PROTO_REallows external modules to access this regular expression, enhancing reusability. The export statement is correctly implemented.
161-165: Addition of 'fetchRequest' and 'fetchResponse' ChannelsThe new channels
fetchRequestandfetchResponseenhance the diagnostics capabilities of the HTTP client. The implementation is correct and consistent with existing patterns.
733-733: Consistent Invocation of 'updateSocketInfo' FunctionAs previously noted,
updateSocketInfois invoked with a different number of arguments compared to line 657. Ensure that this discrepancy is intentional and that the function handles varying argument counts appropriately.
657-657: Verify Consistent Usage of 'updateSocketInfo' FunctionAt line 657,
updateSocketInfois called with two arguments. However, at line 733, it is called with three arguments. Please verify that the function is intended to handle different numbers of arguments and that this usage is correct.To confirm the function signature, run the following script:
✅ Verification successful
Consistent Usage Confirmed for
updateSocketInfoFunctionThe
updateSocketInfofunction is correctly used with both two and three arguments as per its definition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the definition of 'updateSocketInfo' to verify accepted arguments. # Expected result: The function should accept both two and three arguments, or usage should be updated for consistency. # Search for the function definition rg --type js --type ts --hidden --glob '!*node_modules/*' -A 5 'function updateSocketInfo'Length of output: 548
| // const { AsyncLocalStorage } = require('node:async_hooks'); | ||
| import { AsyncLocalStorage } from 'node:async_hooks'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code to improve code clarity
The file contains several blocks of commented-out code (lines 1-2, 6-7, and 20-27) which may no longer be needed. Removing these unused code sections can enhance readability and maintainability.
Also applies to: 6-7, 20-27
| export interface FetchOpaque { | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| [symbols.kRequestId]: number; | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| [symbols.kRequestStartTime]: number; | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| [symbols.kEnableRequestTiming]: number; | ||
| } |
There was a problem hiding this comment.
Avoid using @ts-ignore by properly typing symbol-keyed properties
In the FetchOpaque interface, @ts-ignore comments are used to suppress TypeScript errors when defining properties with symbol keys. Instead of suppressing these errors, consider properly typing the symbol properties to adhere to TypeScript's type system.
For example, you can declare the symbols as unique symbol types:
declare const kRequestId: unique symbol;
declare const kRequestStartTime: unique symbol;
declare const kEnableRequestTiming: unique symbol;
export interface FetchOpaque {
[kRequestId]: number;
[kRequestStartTime]: number;
[kEnableRequestTiming]: number;
}This approach allows TypeScript to recognize the symbol keys without suppressing type checks.
| } | ||
|
|
||
| export function fetchOpaqueInterceptor(opts: OpaqueInterceptorOptions) { | ||
| const opaqueLocalStorage = opts?.opaqueLocalStorage; |
There was a problem hiding this comment.
Remove unnecessary optional chaining for opaqueLocalStorage
Since opaqueLocalStorage is a required property in OpaqueInterceptorOptions, the optional chaining opts?.opaqueLocalStorage in line 33 and opaqueLocalStorage?.getStore() in line 36 is unnecessary. Removing the optional chaining simplifies the code and ensures that undefined values are properly handled elsewhere if needed.
Apply this change:
- const opaqueLocalStorage = opts?.opaqueLocalStorage;
+ const opaqueLocalStorage = opts.opaqueLocalStorage;
...
- const opaque = opaqueLocalStorage?.getStore();
+ const opaque = opaqueLocalStorage.getStore();Also applies to: 36-36
| export function fetchOpaqueInterceptor(opts: OpaqueInterceptorOptions) { | ||
| const opaqueLocalStorage = opts?.opaqueLocalStorage; | ||
| return (dispatch: Dispatcher['dispatch']): Dispatcher['dispatch'] => { | ||
| return function redirectInterceptor(opts: Dispatcher.DispatchOptions, handler: Dispatcher.DispatchHandlers) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename the interceptor function for clarity
The function returned is named redirectInterceptor, which may not accurately reflect its purpose in this context. Consider renaming the function to opaqueInterceptor to better convey its role in handling opaque data.
| return function redirectInterceptor(opts: Dispatcher.DispatchOptions, handler: Dispatcher.DispatchHandlers) { | ||
| const opaque = opaqueLocalStorage?.getStore(); | ||
| (handler as any).opaque = opaque; | ||
| return dispatch(opts, handler); |
There was a problem hiding this comment.
Avoid casting handler to any; extend handler types to include opaque property
Casting handler to any in line 37 ((handler as any).opaque = opaque;) bypasses type safety and can introduce potential bugs. Instead, extend the Dispatcher.DispatchHandlers interface to include the opaque property, ensuring type safety and clarity.
Define a new interface:
interface OpaqueDispatchHandlers extends Dispatcher.DispatchHandlers {
opaque?: FetchOpaque;
}Update the function signature:
return function opaqueInterceptor(
opts: Dispatcher.DispatchOptions,
handler: OpaqueDispatchHandlers
) {
const opaque = opaqueLocalStorage.getStore();
handler.opaque = opaque;
return dispatch(opts, handler);
};This approach maintains type integrity and avoids the pitfalls of using any.
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| import undiciSymbols from 'undici/lib/core/symbols.js'; | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| import undiciFetchSymbols from 'undici/lib/web/fetch/symbols.js'; |
There was a problem hiding this comment.
Avoid using @ts-ignore and importing from internal modules
Importing internal modules from undici, such as 'undici/lib/core/symbols.js' and 'undici/lib/web/fetch/symbols.js', is discouraged because internal modules are not part of the public API and may change without notice, leading to potential breaking changes.
Additionally, using @ts-ignore suppresses TypeScript errors, which can hide potential issues in the code.
Consider accessing the required functionality through the official public API or requesting the library maintainers to expose the necessary symbols.
| export class FetchFactory { | ||
| static #dispatcher: Agent; | ||
| static #opaqueLocalStorage = new AsyncLocalStorage<FetchOpaque>(); | ||
|
|
||
| static getDispatcher() { | ||
| return FetchFactory.#dispatcher ?? getGlobalDispatcher(); | ||
| } | ||
|
|
||
| static setDispatcher(dispatcher: Agent) { | ||
| FetchFactory.#dispatcher = dispatcher; | ||
| } | ||
|
|
||
| static setClientOptions(clientOptions: ClientOptions) { | ||
| let dispatcherOption: Agent.Options = { | ||
| interceptors: { | ||
| Agent: [ | ||
| fetchOpaqueInterceptor({ | ||
| opaqueLocalStorage: FetchFactory.#opaqueLocalStorage, | ||
| }), | ||
| ], | ||
| Client: [], | ||
| }, | ||
| }; | ||
| let dispatcherClazz: new (options: Agent.Options) => Agent = Agent; | ||
| if (clientOptions?.lookup || clientOptions?.checkAddress) { | ||
| dispatcherOption = { | ||
| ...dispatcherOption, | ||
| lookup: clientOptions.lookup, | ||
| checkAddress: clientOptions.checkAddress, | ||
| connect: clientOptions.connect, | ||
| allowH2: clientOptions.allowH2, | ||
| } as HttpAgentOptions; | ||
| dispatcherClazz = HttpAgent as unknown as new (options: Agent.Options) => Agent; | ||
| } else if (clientOptions?.connect) { | ||
| dispatcherOption = { | ||
| ...dispatcherOption, | ||
| connect: clientOptions.connect, | ||
| allowH2: clientOptions.allowH2, | ||
| } as HttpAgentOptions; | ||
| dispatcherClazz = Agent; | ||
| } else if (clientOptions?.allowH2) { | ||
| // Support HTTP2 | ||
| dispatcherOption = { | ||
| ...dispatcherOption, | ||
| allowH2: clientOptions.allowH2, | ||
| } as HttpAgentOptions; | ||
| dispatcherClazz = Agent; | ||
| } | ||
| FetchFactory.#dispatcher = new dispatcherClazz(dispatcherOption); | ||
| initDiagnosticsChannel(); | ||
| } | ||
|
|
||
| static getDispatcherPoolStats() { | ||
| const agent = FetchFactory.getDispatcher(); | ||
| // origin => Pool Instance | ||
| const clients: Map<string, WeakRef<Pool>> | undefined = Reflect.get(agent, undiciSymbols.kClients); | ||
| const poolStatsMap: Record<string, PoolStat> = {}; | ||
| if (!clients) { | ||
| return poolStatsMap; | ||
| } | ||
| for (const [ key, ref ] of clients) { | ||
| const pool = typeof ref.deref === 'function' ? ref.deref() : ref as unknown as Pool; | ||
| const stats = pool?.stats; | ||
| if (!stats) continue; | ||
| poolStatsMap[key] = { | ||
| connected: stats.connected, | ||
| free: stats.free, | ||
| pending: stats.pending, | ||
| queued: stats.queued, | ||
| running: stats.running, | ||
| size: stats.size, | ||
| } satisfies PoolStat; | ||
| } | ||
| return poolStatsMap; | ||
| } | ||
|
|
||
| static async fetch(input: RequestInfo, init?: UrllibRequestInit): Promise<Response> { | ||
| const requestStartTime = performance.now(); | ||
| init = init ?? {}; | ||
| init.dispatcher = init.dispatcher ?? FetchFactory.#dispatcher; | ||
| const request = new Request(input, init); | ||
| const requestId = globalId('HttpClientRequest'); | ||
| // https://developer.chrome.com/docs/devtools/network/reference/?utm_source=devtools#timing-explanation | ||
| const timing = { | ||
| // socket assigned | ||
| queuing: 0, | ||
| // dns lookup time | ||
| // dnslookup: 0, | ||
| // socket connected | ||
| connected: 0, | ||
| // request headers sent | ||
| requestHeadersSent: 0, | ||
| // request sent, including headers and body | ||
| requestSent: 0, | ||
| // Time to first byte (TTFB), the response headers have been received | ||
| waiting: 0, | ||
| // the response body and trailers have been received | ||
| contentDownload: 0, | ||
| }; | ||
|
|
||
| // using opaque to diagnostics channel, binding request and socket | ||
| const internalOpaque = { | ||
| [symbols.kRequestId]: requestId, | ||
| [symbols.kRequestStartTime]: requestStartTime, | ||
| [symbols.kEnableRequestTiming]: !!(init.timing ?? true), | ||
| [symbols.kRequestTiming]: timing, | ||
| // [symbols.kRequestOriginalOpaque]: originalOpaque, | ||
| }; | ||
| const reqMeta: RequestMeta = { | ||
| requestId, | ||
| url: request.url, | ||
| args: { | ||
| method: request.method as HttpMethod, | ||
| type: request.method as HttpMethod, | ||
| data: request.body, | ||
| headers: convertHeader(request.headers), | ||
| }, | ||
| retries: 0, | ||
| }; | ||
| const fetchMeta: FetchMeta = { | ||
| requestId, | ||
| request, | ||
| }; | ||
| const socketInfo: SocketInfo = { | ||
| id: 0, | ||
| localAddress: '', | ||
| localPort: 0, | ||
| remoteAddress: '', | ||
| remotePort: 0, | ||
| remoteFamily: '', | ||
| bytesWritten: 0, | ||
| bytesRead: 0, | ||
| handledRequests: 0, | ||
| handledResponses: 0, | ||
| }; | ||
| channels.request.publish({ | ||
| request: reqMeta, | ||
| } as RequestDiagnosticsMessage); | ||
| channels.fetchRequest.publish({ | ||
| fetch: fetchMeta, | ||
| } as FetchDiagnosticsMessage); | ||
|
|
||
| let res: Response; | ||
| // keep urllib createCallbackResponse style | ||
| const resHeaders: IncomingHttpHeaders = {}; | ||
| const urllibResponse = { | ||
| status: -1, | ||
| statusCode: -1, | ||
| statusText: '', | ||
| statusMessage: '', | ||
| headers: resHeaders, | ||
| size: 0, | ||
| aborted: false, | ||
| rt: 0, | ||
| keepAliveSocket: true, | ||
| requestUrls: [ | ||
| request.url, | ||
| ], | ||
| timing, | ||
| socket: socketInfo, | ||
| retries: 0, | ||
| socketErrorRetries: 0, | ||
| } as any as RawResponseWithMeta; | ||
| try { | ||
| await FetchFactory.#opaqueLocalStorage.run(internalOpaque, async () => { | ||
| res = await UndiciFetch(input, init); | ||
| }); | ||
| } catch (e: any) { | ||
| channels.response.publish({ | ||
| fetch: fetchMeta, | ||
| error: e, | ||
| } as FetchResponseDiagnosticsMessage); | ||
| channels.fetchResponse.publish({ | ||
| request: reqMeta, | ||
| response: urllibResponse, | ||
| error: e, | ||
| } as ResponseDiagnosticsMessage); | ||
| throw e; | ||
| } | ||
|
|
||
| // get unidici internal response | ||
| const state = Reflect.get(res!, undiciFetchSymbols.kState) as Dispatcher.ResponseData; | ||
| updateSocketInfo(socketInfo, internalOpaque /* , rawError */); | ||
|
|
||
| urllibResponse.headers = convertHeader(res!.headers); | ||
| urllibResponse.status = urllibResponse.statusCode = res!.status; | ||
| urllibResponse!.statusMessage = res!.statusText; | ||
| if (urllibResponse.headers['content-length']) { | ||
| urllibResponse.size = parseInt(urllibResponse.headers['content-length']); | ||
| } | ||
| urllibResponse.rt = performanceTime(requestStartTime); | ||
|
|
||
| channels.fetchResponse.publish({ | ||
| fetch: fetchMeta, | ||
| timingInfo: (state as any).timingInfo, | ||
| response: res!, | ||
| } as FetchResponseDiagnosticsMessage); | ||
| channels.response.publish({ | ||
| request: reqMeta, | ||
| response: urllibResponse, | ||
| } as ResponseDiagnosticsMessage); | ||
| return res!; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor FetchFactory class to use plain functions instead of static methods
The FetchFactory class contains only static methods and properties. According to best practices, it's preferable to use modules or namespaces when you don't need to instantiate a class. Classes with only static members can add unnecessary complexity and may confuse other developers about the intended use.
Consider refactoring the code by exporting plain functions and constants without wrapping them in a class.
🧰 Tools
🪛 Biome
[error] 59-262: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🪛 GitHub Check: codecov/patch
[warning] 68-69: src/fetch.ts#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 84-91: src/fetch.ts#L84-L91
Added lines #L84 - L91 were not covered by tests
[warning] 93-98: src/fetch.ts#L93-L98
Added lines #L93 - L98 were not covered by tests
[warning] 101-106: src/fetch.ts#L101-L106
Added lines #L101 - L106 were not covered by tests
[warning] 117-118: src/fetch.ts#L117-L118
Added lines #L117 - L118 were not covered by tests
[warning] 227-237: src/fetch.ts#L227-L237
Added lines #L227 - L237 were not covered by tests
| channels.response.publish({ | ||
| fetch: fetchMeta, | ||
| error: e, | ||
| } as FetchResponseDiagnosticsMessage); | ||
| channels.fetchResponse.publish({ | ||
| request: reqMeta, | ||
| response: urllibResponse, | ||
| error: e, | ||
| } as ResponseDiagnosticsMessage); | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
Add unit tests for error handling in the fetch method
The error handling block in the fetch method is crucial for managing exceptions during the fetch operation and publishing diagnostic messages. Currently, this logic is not covered by unit tests.
Adding tests that simulate errors during the fetch process will ensure that exceptions are handled correctly and that diagnostic messages are published as expected, enhancing the robustness of the code.
Would you like assistance in creating unit tests for these error scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 227-237: src/fetch.ts#L227-L237
Added lines #L227 - L237 were not covered by tests
| const state = Reflect.get(res!, undiciFetchSymbols.kState) as Dispatcher.ResponseData; | ||
| updateSocketInfo(socketInfo, internalOpaque /* , rawError */); |
There was a problem hiding this comment.
Avoid accessing internal properties of external libraries
Using Reflect.get to access internal properties like undiciFetchSymbols.kState can lead to fragile code because these internal implementations may change without warning in future library updates.
Consider using the public API of the undici library to achieve the desired functionality or reaching out to the library maintainers to expose the necessary properties officially.
| export interface UnidiciTimingInfo { | ||
| startTime: number; | ||
| redirectStartTime: number; | ||
| redirectEndTime: number; | ||
| postRedirectStartTime: number; | ||
| finalServiceWorkerStartTime: number; | ||
| finalNetworkResponseStartTime: number; | ||
| finalNetworkRequestStartTime: number; | ||
| endTime: number; | ||
| encodedBodySize: number; | ||
| decodedBodySize: number; | ||
| finalConnectionTimingInfo: { | ||
| domainLookupStartTime: number; | ||
| domainLookupEndTime: number; | ||
| connectionStartTime: number; | ||
| connectionEndTime: number; | ||
| secureConnectionStartTime: number; | ||
| // ALPNNegotiatedProtocol: undefined | ||
| }; | ||
| } |
There was a problem hiding this comment.
Correct Typo in Interface Name to 'UndiciTimingInfo'
The interface name UnidiciTimingInfo seems to have a typo. It should be UndiciTimingInfo to match the 'undici' module name.
Apply this diff to correct the interface name:
-export interface UnidiciTimingInfo {
+export interface UndiciTimingInfo {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface UnidiciTimingInfo { | |
| startTime: number; | |
| redirectStartTime: number; | |
| redirectEndTime: number; | |
| postRedirectStartTime: number; | |
| finalServiceWorkerStartTime: number; | |
| finalNetworkResponseStartTime: number; | |
| finalNetworkRequestStartTime: number; | |
| endTime: number; | |
| encodedBodySize: number; | |
| decodedBodySize: number; | |
| finalConnectionTimingInfo: { | |
| domainLookupStartTime: number; | |
| domainLookupEndTime: number; | |
| connectionStartTime: number; | |
| connectionEndTime: number; | |
| secureConnectionStartTime: number; | |
| // ALPNNegotiatedProtocol: undefined | |
| }; | |
| } | |
| export interface UndiciTimingInfo { | |
| startTime: number; | |
| redirectStartTime: number; | |
| redirectEndTime: number; | |
| postRedirectStartTime: number; | |
| finalServiceWorkerStartTime: number; | |
| finalNetworkResponseStartTime: number; | |
| finalNetworkRequestStartTime: number; | |
| endTime: number; | |
| encodedBodySize: number; | |
| decodedBodySize: number; | |
| finalConnectionTimingInfo: { | |
| domainLookupStartTime: number; | |
| domainLookupEndTime: number; | |
| connectionStartTime: number; | |
| connectionEndTime: number; | |
| secureConnectionStartTime: number; | |
| // ALPNNegotiatedProtocol: undefined | |
| }; | |
| } |
[skip ci] ## [4.4.0](v4.3.1...v4.4.0) (2024-10-08) ### Features * impl fetch ([#542](#542)) ([55a634c](55a634c))
2 participants
Summary by CodeRabbit
Release Notes
New Features
FetchFactoryclass for enhanced HTTP request management with diagnostics.FetchOpaquetype for tracking request metadata.HttpClientwith new timing and diagnostics capabilities.Bug Fixes
HttpClient.Documentation
Tests
fetchfunctionality to ensure reliability.