Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: impl fetch #542

Merged
merged 2 commits into from
Oct 8, 2024
Merged

feat: impl fetch #542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cb5f25
feat: impl fetch
killagu Oct 8, 2024
b1e131f
f
killagu Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/FetchOpaqueInterceptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// const { AsyncLocalStorage } = require('node:async_hooks');
import { AsyncLocalStorage } from 'node:async_hooks';
Comment on lines +1 to +2
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

import symbols from './symbols.js';
import { Dispatcher } from 'undici';

// const RedirectHandler = require('../handler/redirect-handler')

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;
}
Comment on lines +8 to +18
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.


// const internalOpaque = {
// [symbols.kRequestId]: requestId,
// [symbols.kRequestStartTime]: requestStartTime,
// [symbols.kEnableRequestTiming]: !!(init.timing ?? true),
// [symbols.kRequestTiming]: timing,
// // [symbols.kRequestOriginalOpaque]: originalOpaque,
// };

export interface OpaqueInterceptorOptions {
opaqueLocalStorage: AsyncLocalStorage<FetchOpaque>;
}

export function fetchOpaqueInterceptor(opts: OpaqueInterceptorOptions) {
const opaqueLocalStorage = opts?.opaqueLocalStorage;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

return (dispatch: Dispatcher['dispatch']): Dispatcher['dispatch'] => {
return function redirectInterceptor(opts: Dispatcher.DispatchOptions, handler: Dispatcher.DispatchHandlers) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

const opaque = opaqueLocalStorage?.getStore();
(handler as any).opaque = opaque;
return dispatch(opts, handler);
Comment on lines +35 to +38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

};
};
}
14 changes: 8 additions & 6 deletions src/HttpAgent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ import {

export type CheckAddressFunction = (ip: string, family: number | string, hostname: string) => boolean;

export type HttpAgentOptions = {
export interface HttpAgentOptions extends Agent.Options {
lookup?: LookupFunction;
checkAddress?: CheckAddressFunction;
connect?: buildConnector.BuildOptions,
allowH2?: boolean;
};
}

class IllegalAddressError extends Error {
hostname: string;
Expand All @@ -36,9 +36,10 @@ export class HttpAgent extends Agent {

constructor(options: HttpAgentOptions) {
/* eslint node/prefer-promises/dns: off*/
const _lookup = options.lookup ?? dns.lookup;
const lookup: LookupFunction = (hostname, dnsOptions, callback) => {
_lookup(hostname, dnsOptions, (err, ...args: any[]) => {
const { lookup = dns.lookup, ...baseOpts } = options;

const lookupFunction: LookupFunction = (hostname, dnsOptions, callback) => {
lookup(hostname, dnsOptions, (err, ...args: any[]) => {
// address will be array on Node.js >= 20
const address = args[0];
const family = args[1];
Expand All @@ -63,7 +64,8 @@ export class HttpAgent extends Agent {
});
};
super({
connect: { ...options.connect, lookup, allowH2: options.allowH2 },
...baseOpts,
connect: { ...options.connect, lookup: lookupFunction, allowH2: options.allowH2 },
});
this.#checkAddress = options.checkAddress;
}
Expand Down
69 changes: 28 additions & 41 deletions src/HttpClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { HttpAgent, CheckAddressFunction } from './HttpAgent.js';
import type { IncomingHttpHeaders } from './IncomingHttpHeaders.js';
import { RequestURL, RequestOptions, HttpMethod, RequestMeta } from './Request.js';
import { RawResponseWithMeta, HttpClientResponse, SocketInfo } from './Response.js';
import { parseJSON, digestAuthHeader, globalId, performanceTime, isReadable } from './utils.js';
import { parseJSON, digestAuthHeader, globalId, performanceTime, isReadable, updateSocketInfo } from './utils.js';
import symbols from './symbols.js';
import { initDiagnosticsChannel } from './diagnosticsChannel.js';
import { HttpClientConnectTimeoutError, HttpClientRequestTimeoutError } from './HttpClientError.js';
Expand All @@ -47,7 +47,28 @@ type UndiciRequestOption = Exists<Parameters<typeof undiciRequest>[1]>;
type PropertyShouldBe<T, K extends keyof T, V> = Omit<T, K> & { [P in K]: V };
type IUndiciRequestOption = PropertyShouldBe<UndiciRequestOption, 'headers', IncomingHttpHeaders>;

const PROTO_RE = /^https?:\/\//i;
export const PROTO_RE = /^https?:\/\//i;

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
};
}
Comment on lines +52 to +71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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
};
}


function noop() {
// noop
Expand Down Expand Up @@ -137,9 +158,11 @@ export type RequestContext = {
requestStartTime?: number;
};

const channels = {
export const channels = {
request: diagnosticsChannel.channel('urllib:request'),
response: diagnosticsChannel.channel('urllib:response'),
fetchRequest: diagnosticsChannel.channel('urllib:fetch:request'),
fetchResponse: diagnosticsChannel.channel('urllib:fetch:response'),
};

export type RequestDiagnosticsMessage = {
Expand Down Expand Up @@ -631,7 +654,7 @@ export class HttpClient extends EventEmitter {
}
res.rt = performanceTime(requestStartTime);
// get real socket info from internalOpaque
this.#updateSocketInfo(socketInfo, internalOpaque);
updateSocketInfo(socketInfo, internalOpaque);

const clientResponse: HttpClientResponse = {
opaque: originalOpaque,
Expand Down Expand Up @@ -707,7 +730,7 @@ export class HttpClient extends EventEmitter {
res.requestUrls.push(requestUrl.href);
}
res.rt = performanceTime(requestStartTime);
this.#updateSocketInfo(socketInfo, internalOpaque, rawError);
updateSocketInfo(socketInfo, internalOpaque, rawError);

channels.response.publish({
request: reqMeta,
Expand All @@ -729,40 +752,4 @@ export class HttpClient extends EventEmitter {
throw err;
}
}

#updateSocketInfo(socketInfo: SocketInfo, internalOpaque: any, err?: any) {
const socket = internalOpaque[symbols.kRequestSocket] ?? err?.[symbols.kErrorSocket];
if (socket) {
socketInfo.id = socket[symbols.kSocketId];
socketInfo.handledRequests = socket[symbols.kHandledRequests];
socketInfo.handledResponses = socket[symbols.kHandledResponses];
if (socket[symbols.kSocketLocalAddress]) {
socketInfo.localAddress = socket[symbols.kSocketLocalAddress];
socketInfo.localPort = socket[symbols.kSocketLocalPort];
}
if (socket.remoteAddress) {
socketInfo.remoteAddress = socket.remoteAddress;
socketInfo.remotePort = socket.remotePort;
socketInfo.remoteFamily = socket.remoteFamily;
}
socketInfo.bytesRead = socket.bytesRead;
socketInfo.bytesWritten = socket.bytesWritten;
if (socket[symbols.kSocketConnectErrorTime]) {
socketInfo.connectErrorTime = socket[symbols.kSocketConnectErrorTime];
if (Array.isArray(socket.autoSelectFamilyAttemptedAddresses)) {
socketInfo.attemptedRemoteAddresses = socket.autoSelectFamilyAttemptedAddresses;
}
socketInfo.connectProtocol = socket[symbols.kSocketConnectProtocol];
socketInfo.connectHost = socket[symbols.kSocketConnectHost];
socketInfo.connectPort = socket[symbols.kSocketConnectPort];
}
if (socket[symbols.kSocketConnectedTime]) {
socketInfo.connectedTime = socket[symbols.kSocketConnectedTime];
}
if (socket[symbols.kSocketRequestEndTime]) {
socketInfo.lastRequestEndTime = socket[symbols.kSocketRequestEndTime];
}
socket[symbols.kSocketRequestEndTime] = new Date();
}
}
}
6 changes: 6 additions & 0 deletions src/Request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { EventEmitter } from 'node:events';
import type { Dispatcher } from 'undici';
import type { IncomingHttpHeaders } from './IncomingHttpHeaders.js';
import type { HttpClientResponse } from './Response.js';
import { Request } from 'undici';

export type HttpMethod = Dispatcher.HttpMethod;

Expand Down Expand Up @@ -161,3 +162,8 @@ export type RequestMeta = {
ctx?: unknown;
retries: number;
};

export type FetchMeta = {
requestId: number;
request: Request,
};
Loading
Loading