Allow custom fetch and remove undici-specific dispatcher option #40

Corvince · 2025-11-19T09:48:17Z

Updated the fetch path options to

Allow custom fetch implementations, which could be from a different package or simply a wrapper around fetch
Remove explicit dispatcher option, which is a undici (and thus nodejs) specific requestoption. Dispatcher should now be set under the reqular requestOptions, and users must make sure that it is valid fetch option.

Copilot

Pull Request Overview

This PR refactors the fetch configuration API to support custom fetch implementations and remove the undici-specific dispatcher option from the top level of FetchOptions.

Key Changes:

Separates fetch (custom fetch function) from fetchOptions (configuration object) in the ServerOptions interface
Moves the dispatcher property from a dedicated field in FetchOptions to be part of the standard requestOptions
Updates all tests and documentation to reflect the new API structure

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`lib/http-proxy/index.ts`	Refactors TypeScript interfaces to split `fetch` option into `fetchOptions` (configuration) and `fetch` (custom fetch function), removes `Dispatcher` type
`lib/http-proxy/passes/web-incoming.ts`	Updates implementation to use custom fetch and retrieve dispatcher from `requestOptions` instead of dedicated property
`lib/test/http/proxy-http2-to-http2.test.ts`	Migrates test to use new `fetchOptions.requestOptions.dispatcher` structure
`lib/test/http/proxy-callbacks.test.ts`	Migrates test to use new `fetchOptions.requestOptions.dispatcher` structure
`lib/test/http/customFetch.test.ts`	Adds new test demonstrating custom fetch implementation usage
`README.md`	Updates all examples and documentation to reflect the new API structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-20T19:26:58Z

README.md

 const proxy = createProxyServer({
  target: "https://http2-server.example.com",
-  fetch: true  // Uses default fetch configuration
+  fetch  // Uses default fetch configuration


The example is incomplete or unclear. Based on the PR changes, fetch should be set to the global fetch function to use default fetch configuration. This should be fetch: fetch or just importing and using the global fetch. The current syntax fetch without assignment is ambiguous and may not work as intended.

seems fine to me as is.

Copilot · 2025-11-20T19:26:58Z

README.md

- **fetch**: Enable fetch API for HTTP/2 support. Set to `true` for defaults, or provide custom configuration:
-  - `dispatcher`: Custom fetch dispatcher (e.g., undici Agent with `allowH2: true` for HTTP/2)
-  - `requestOptions`: Additional fetch request options
+- **fetchOptions**: Enable fetch API for HTTP/2 support. Set to `true` for defaults, or provide custom configuration:


The documentation is incorrect. fetchOptions cannot be set to true according to the new TypeScript interface definition. It should be an object of type FetchOptions or undefined. Remove the "Set to true for defaults, or" part, or update the interface to support true as a value.

Suggested change

- **fetchOptions**: Enable fetch API for HTTP/2 support. Set to `true` for defaults, or provide custom configuration:

- **fetchOptions**: Enable fetch API for HTTP/2 support. Provide an object of type `FetchOptions` for custom configuration:

It has a very good point.

Copilot · 2025-11-20T19:26:59Z

lib/test/http/customFetch.test.ts

@@ -0,0 +1,70 @@
+/*
+pnpm test proxy-http2-to-http2.test.ts


The comment references the wrong test file. It should reference customFetch.test.ts instead of proxy-http2-to-http2.test.ts.

Suggested change

pnpm test proxy-http2-to-http2.test.ts

pnpm test customFetch.test.ts

Copilot · 2025-11-20T19:26:59Z

lib/test/http/customFetch.test.ts

+  return fetch(url, {...options, dispatcher: TestAgent as any });
+};
+
+describe("Basic example of proxying over HTTP2 to a target HTTP2 with custom Fetch option server", () => {


The test description has awkward wording with "custom Fetch option server" at the end. It should be "with custom Fetch option" or "server with custom Fetch option".

Suggested change

describe("Basic example of proxying over HTTP2 to a target HTTP2 with custom Fetch option server", () => {

describe("Basic example of proxying over HTTP2 to a target HTTP2 server with custom Fetch option", () => {

fine either way

Copilot · 2025-11-20T19:26:59Z

lib/http-proxy/index.ts

   * This is passed to https.request.
   */
  ca?: string;
  /** Enable using fetch for proxy requests. Set to true for defaults, or provide custom configuration. */


The JSDoc comment is outdated. It mentions "Set to true for defaults, or provide custom configuration", but the fetchOptions property only accepts FetchOptions type, not boolean. The comment should be updated to reflect the new API design where fetchOptions is an optional object and fetch is a separate property for custom fetch implementations.

Suggested change

/** Enable using fetch for proxy requests. Set to true for defaults, or provide custom configuration. */

/** Optional configuration object for fetch-based proxy requests. Use this to customize fetch request and response handling. For custom fetch implementations, use the `fetch` property. */

Copilot · 2025-11-20T19:27:00Z

lib/http-proxy/index.ts

  ca?: string;
  /** Enable using fetch for proxy requests. Set to true for defaults, or provide custom configuration. */
-  fetch?: boolean | FetchOptions;
+  fetchOptions?: FetchOptions;


Missing JSDoc comment for the new fetch property. This should document that it allows providing a custom fetch implementation (e.g., a wrapper around fetch or from a different package).

Suggested change

fetchOptions?: FetchOptions;

fetchOptions?: FetchOptions;

/**

* Allows providing a custom fetch implementation (e.g., a wrapper around fetch or from a different package).

* If set, this function will be used for proxy requests instead of the default fetch.

*/

seems fine as you have it, since the comment applies to both options (fetch and fetchOptions).

Copilot · 2025-11-20T19:27:00Z

lib/http-proxy/passes/web-incoming.ts

  server.emit("start", req, res, options.target || options.forward!);

-  if (options.fetch || process.env.FORCE_FETCH_PATH === "true") {
+  if (options.fetch ||options.fetchOptions || process.env.FORCE_FETCH_PATH === "true") {


Missing space after || operator. Should be options.fetch || options.fetchOptions.

Suggested change

if (options.fetch ||options.fetchOptions || process.env.FORCE_FETCH_PATH === "true") {

if (options.fetch || options.fetchOptions || process.env.FORCE_FETCH_PATH === "true") {

i'll run prettier so don't care.

williamstein · 2025-11-20T19:36:10Z

The code looks good to me, but the AI did have some valid concerns about some comments/docs.

vhess added 4 commits November 18, 2025 16:40

Add custom Fetch option and supersede dispatcher option

a13d600

rename customFetch to fetch and address some documentation errors

e776e7b

update custom fetch with new naming

c15085e

fix: resolve type errors (with shameful any type)

28703b8

williamstein requested a review from Copilot November 20, 2025 19:23

Copilot started reviewing on behalf of williamstein November 20, 2025 19:23 View session

Copilot finished reviewing on behalf of williamstein November 20, 2025 19:25

Copilot AI reviewed Nov 20, 2025

View reviewed changes

williamstein merged commit d8ca477 into sagemathinc:main Nov 20, 2025
9 checks passed

	- fetchOptions: Enable fetch API for HTTP/2 support. Set to `true` for defaults, or provide custom configuration:
	- fetchOptions: Enable fetch API for HTTP/2 support. Provide an object of type `FetchOptions` for custom configuration:

	pnpm test proxy-http2-to-http2.test.ts
	pnpm test customFetch.test.ts

	describe("Basic example of proxying over HTTP2 to a target HTTP2 with custom Fetch option server", () => {
	describe("Basic example of proxying over HTTP2 to a target HTTP2 server with custom Fetch option", () => {

	/** Enable using fetch for proxy requests. Set to true for defaults, or provide custom configuration. */
	/** Optional configuration object for fetch-based proxy requests. Use this to customize fetch request and response handling. For custom fetch implementations, use the `fetch` property. */

	if (options.fetch \|\|options.fetchOptions \|\| process.env.FORCE_FETCH_PATH === "true") {
	if (options.fetch \|\| options.fetchOptions \|\| process.env.FORCE_FETCH_PATH === "true") {

Allow custom fetch and remove undici-specific dispatcher option #40

Allow custom fetch and remove undici-specific dispatcher option #40

Uh oh!

Conversation

Corvince commented Nov 19, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

williamstein commented Nov 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants