feat!: replace make-fetch-happen with ky and refresh tests/deps #784

gastonfournier · 2025-11-26T15:20:03Z

About the changes

Made HTTP retry count configurable via httpOptions.maxRetries (defaults unchanged) and switched httpOptions.agent → dispatcher to align with undici dispatchers (src/request.ts, src/http-options.ts).
Updated ky client setup so the retry limit comes from httpOptions.maxRetries, and removed per-request retry overrides to let the shared client control retries (src/request.ts).
Re-enabled the 429 backoff tests and set httpOptions: { maxRetries: 0 } so they run fast without extra retry noise; adjusted nock setups accordingly (src/test/repository/repository.test.ts).

coveralls · 2025-12-01T12:52:41Z

Pull Request Test Coverage Report for Build 20067960829

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

65 of 71 (91.55%) changed or added relevant lines in 6 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.6%) to 89.277%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/request.ts	40	41	97.56%
src/client-spec-version.ts	10	15	66.67%

Files with Coverage Reduction	New Missed Lines	%
src/repository/polling-fetcher.ts	3	83.85%

Totals
Change from base Build 19896466463:	-0.6%
Covered Lines:	1226
Relevant Lines:	1318

💛 - Coveralls

This reverts commit b5882b0.

- add undici agents/dispatcher bridge so ky honors agent options per call - preserve rejectUnauthorized and proxy support without global env mutation - keep ky throwing on HTTP errors and normalize to Response via helper - remove unused http/https-proxy-agent and update request agent tests to undici

gastonfournier · 2025-12-05T16:45:26Z

src/repository/bootstrap-provider.ts

-        custom: this.urlHeaders,
-      }),
-      retry: {
-        retries: 2,


2 is default retry

gastonfournier · 2025-12-05T23:08:27Z

src/repository/streaming-fetcher.ts

        etag: undefined,
        contentType: undefined,
-        custom: this.headers,
-        specVersionSupported: '5.2.0',


This was always out of sync so I added client-spec-version.ts helper (please review it)

gastonfournier · 2025-12-05T23:10:24Z

src/metrics.ts

+      appName,
+      instanceId,
+      connectionId,
+      timeout,
+      httpOptions,


Turned this into an immutable configuration per client. Maybe we should still enable different timeouts per request, and perhaps some httpOptions, but I also don't think we need them

Copilot

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.

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

Copilot · 2025-12-05T23:12:38Z

src/request.ts

+          connectionId,
+          interval,
+          etag: undefined,
+          contentType: 'application/json',


Consider removing the explicit contentType: 'application/json' assignment and instead relying on ky's default behavior for JSON requests. When using the json option in ky, it automatically sets the Content-Type header to application/json, making this explicit assignment redundant.

Suggested change

contentType: 'application/json',

I think this is fine

Copilot · 2025-12-05T23:12:39Z

src/request.ts

+          interval,
+          etag,
+          headers,
+          connectionId,


Missing contentType parameter in the buildHeaders call. While not required for GET requests, it's inconsistent with the POST request implementation. Consider explicitly passing contentType: undefined for clarity and consistency:

headers: buildHeaders({ appName, instanceId, interval, etag, headers, connectionId, contentType: undefined, }),

Suggested change

connectionId,

connectionId,

contentType: undefined,

Copilot · 2025-12-05T23:12:39Z

src/http-options.ts


 export interface HttpOptions {
-  agent?: (url: URL) => Agent;
+  dispatcher?: (url: URL) => Dispatcher; // this is a breaking change from 'agent'. Ref: https://github.com/Unleash/unleash-node-sdk/pull/332


Breaking change documentation: While the inline comment mentions this is a breaking change and references PR #332, consider documenting this breaking change more prominently in the PR description or changelog. The change from agent (which used Node.js http/https Agent) to dispatcher (which uses undici Dispatcher) requires users to update their code.

Additionally, consider adding a deprecation period or providing a migration guide for users who were using the agent option.

Suggested change

dispatcher?: (url: URL) => Dispatcher; // this is a breaking change from 'agent'. Ref: https://github.com/Unleash/unleash-node-sdk/pull/332

/**

* @deprecated Use `dispatcher` instead. The `agent` option (Node.js http/https Agent) has been replaced by `dispatcher` (undici Dispatcher).

* See migration guide: https://github.com/Unleash/unleash-node-sdk/pull/332

*/

agent?: any;

/**

* Custom dispatcher for HTTP requests.

* This replaces the deprecated `agent` option.

* See migration guide: https://github.com/Unleash/unleash-node-sdk/pull/332

*/

dispatcher?: (url: URL) => Dispatcher;

Copilot · 2025-12-05T23:12:40Z

src/test/unleash.network.retries.test.ts

-        expect(calls).toBe(3);
+        if (finished || calls < expectedCalls) return;
+        finished = true;
+        expect(calls).toBeGreaterThanOrEqual(expectedCalls);


The test assertion uses toBeGreaterThanOrEqual instead of a strict equality check. While this makes the test more resilient, it also makes it less precise. Consider using toBe(expectedCalls) if you want to verify exact retry behavior:

expect(calls).toBe(expectedCalls);

This would ensure that retries are working exactly as configured (2 retries + 1 initial = 3 total).

src/request.ts

gastonfournier · 2025-12-05T23:13:16Z

src/request.ts

+const getDefaultAgent = (url: URL, rejectUnauthorized?: boolean): Dispatcher => {
+  const proxy = getProxyForUrl(url.href);
+  const connect = rejectUnauthorized === undefined ? undefined : { rejectUnauthorized };
+
+  if (!proxy || proxy === '') {
+    return new UndiciAgent({ connect });
+  }
+
+  return new ProxyAgent({ uri: proxy, connect });
+};
+
+const createFetchWithDispatcher =
+  (httpOptions?: HttpOptions): typeof fetch =>
+  (input: string | URL | globalThis.Request, init?: RequestInit) => {
+    const resolveDispatcher =
+      httpOptions?.dispatcher ||
+      ((targetUrl: URL) => getDefaultAgent(targetUrl, httpOptions?.rejectUnauthorized));
+
+    const getUrl = (): URL =>
+      typeof input === 'string' || input instanceof URL ? new URL(input) : new URL(input.url);
+
+    const dispatcher = resolveDispatcher(getUrl());
+
+    return fetch(input, {
+      ...(init ?? {}),
+      // @ts-expect-error - dispatcher is not part of RequestInit, but undici accepts it.
+      dispatcher,
+    });
+  };


Not very happy with these AI shenanigans... It's supposed to be replicating rejectUnauthorized behavior from make-fetch-happen. And maintaining the ability to inject an agent (now Dispatcher), introduced in #332

If we refactor this part and/or decide to drop #332 I'd be ok.

gastonfournier · 2025-12-05T23:14:17Z

src/request.ts

+interface RequestOptions {
  url: string;
  timeout?: number;
+  interval?: number;


Something for a future refactor, why would the interval be bound to a request? The only explanation is that we have in on a header, but I resisted from refactoring this part

feat: replace make-fetch-happen with ky and refresh tests/deps

0048dc6

gastonfournier added the 🤖👤 mostly-ai LLM-led with human assistance label Nov 26, 2025

gastonfournier self-assigned this Nov 26, 2025

thomasheartman added this to Issues and PRs Nov 26, 2025

github-project-automation bot moved this to New in Issues and PRs Nov 26, 2025

Revert unnecessary changes

7e5149b

gastonfournier force-pushed the ky-migration branch from 783b433 to 7e5149b Compare November 26, 2025 15:34

gastonfournier changed the title ~~feat: replace make-fetch-happen with ky and refresh tests/deps~~ feat!: replace make-fetch-happen with ky and refresh tests/deps Nov 26, 2025

chore: break support with node 16 and include node LTS: 24

09c2cf5

gastonfournier moved this from New to Bots in Issues and PRs Nov 27, 2025

gastonfournier moved this from Bots to Todo in Issues and PRs Nov 27, 2025

gastonfournier and others added 3 commits December 1, 2025 13:42

feat: replace make-fetch-happen with ky and refresh tests/deps

4b527b2

Revert unnecessary changes

dd74dcb

fix: missing linting after rebasing from main

15b82cd

chriswk force-pushed the ky-migration branch from 09c2cf5 to 15b82cd Compare December 1, 2025 12:47

gastonfournier added 2 commits December 4, 2025 18:38

Merged from main

cb6875f

Fixes after merge

6ffc01c

Copilot AI review requested due to automatic review settings December 4, 2025 17:46

Copilot started reviewing on behalf of gastonfournier December 4, 2025 17:47 View session

Keep node 18

b5882b0

Copilot finished reviewing on behalf of gastonfournier December 4, 2025 17:50

Revert "Keep node 18"

58eea3c

This reverts commit b5882b0.

This comment was marked as outdated.

Sign in to view

gastonfournier added 2 commits December 4, 2025 19:06

Test fix

7b59ffe

Some improvements suggested by copilot

3ff32ad

gastonfournier requested a review from Copilot December 4, 2025 18:19

Copilot started reviewing on behalf of gastonfournier December 4, 2025 18:19 View session

Copilot finished reviewing on behalf of gastonfournier December 4, 2025 18:21

Remove behavior test

f2864c6

This comment was marked as outdated.

Sign in to view

gastonfournier requested a review from Copilot December 4, 2025 18:50

Copilot started reviewing on behalf of gastonfournier December 4, 2025 18:50 View session

Copilot finished reviewing on behalf of gastonfournier December 4, 2025 18:53

This comment was marked as outdated.

Sign in to view

gastonfournier added 2 commits December 5, 2025 11:51

Refactor types

4a71bc6

Refactor to reduce verbosity and param handling

b21b89a

gastonfournier commented Dec 5, 2025

View reviewed changes

src/repository/bootstrap-provider.ts

custom: this.urlHeaders,

}),

retry: {

retries: 2,

Copy link

Contributor Author

gastonfournier Dec 5, 2025

Choose a reason for hiding this comment

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

2 is default retry

gastonfournier added 4 commits December 5, 2025 20:24

Standardize headers param

cd8d6e3

Set max retries configurable to use it in tests

c7371ae

Use dispatcher instead of agent

ddb41ac

Attempt to fix tests by avoiding override retries

2c2f517

gastonfournier requested a review from Copilot December 5, 2025 23:05

Copilot started reviewing on behalf of gastonfournier December 5, 2025 23:06 View session

gastonfournier commented Dec 5, 2025

View reviewed changes

Copilot AI reviewed Dec 5, 2025

View reviewed changes

gastonfournier commented Dec 5, 2025

View reviewed changes

gastonfournier added 👤🤝🤖 half-half Roughly equal human-LLM collaboration and removed 🤖👤 mostly-ai LLM-led with human assistance labels Dec 5, 2025

Keep CommonJS compatibility

6ce22fe

-  dispatcher?: (url: URL) => Dispatcher; // this is a breaking change from 'agent'. Ref: https://github.com/Unleash/unleash-node-sdk/pull/332
+  /**
+   * @deprecated Use `dispatcher` instead. The `agent` option (Node.js http/https Agent) has been replaced by `dispatcher` (undici Dispatcher).
+   * See migration guide: https://github.com/Unleash/unleash-node-sdk/pull/332
+   */
+  agent?: any;
+  /**
+   * Custom dispatcher for HTTP requests.
+   * This replaces the deprecated `agent` option.
+   * See migration guide: https://github.com/Unleash/unleash-node-sdk/pull/332
+   */
+  dispatcher?: (url: URL) => Dispatcher;

feat!: replace make-fetch-happen with ky and refresh tests/deps #784

Are you sure you want to change the base?

feat!: replace make-fetch-happen with ky and refresh tests/deps #784

Uh oh!

Conversation

gastonfournier commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About the changes

Uh oh!

coveralls commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 20067960829

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

gastonfournier Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

gastonfournier Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

gastonfournier Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

gastonfournier Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gastonfournier Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

gastonfournier Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gastonfournier commented Nov 26, 2025 •

edited

Loading

coveralls commented Dec 1, 2025 •

edited

Loading