Conversation
davidlehn
force-pushed
the
remove-cjs-support
branch
2 times, most recently
from
August 11, 2026 01:17
bb0518d to
cc1f63d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## update-deps #56 +/- ##
=================================================
+ Coverage 89.64% 100.00% +10.35%
=================================================
Files 3 4 +1
Lines 309 86 -223
=================================================
- Hits 277 86 -191
+ Misses 32 0 -32
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ky@2 merges header options via a plain object spread when both sides are still plain objects, which does not dedupe names that differ only by case (e.g. `Accept` vs `accept`), causing values to be appended instead of overridden. Use a `Headers` instance instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Fix import. - Use suggested ChromeHeadless options for CI.
Webpack's browser field remaps `tests/utils.js` to `tests/utils-browser.js`, but the browser stub never defined `makeAgent`. The namespace import in the shared spec file only references it inside an `isNode` guard, but webpack still statically validates the export, breaking the karma build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adding a non-simple header (e.g. `Authorization`) triggers a browser CORS preflight `OPTIONS` request, which this route never answered, causing the actual request to be blocked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`ky@2` wraps the browser's `TypeError: Failed to fetch` in its own `NetworkError`, with the original error moved to `cause`. Check both locations so the friendly CORS message still gets applied. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Launch the browser with `--ignore-certificate-errors` so it accepts the self-signed cert, letting the local HTTPS test server test run in both node and browsers. This covers TLS in the browser without depending on an external site. Restrict the github.com test to node. The site sends no CORS headers, so a browser blocks the request before it is sent. Keeping it node-only also halves how often it runs, reducing rate limit exposure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`engines` requires node >=22, so the node 18.2+ guard on agent conversion is always true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Aligns the installed undici with the one built into the current Node.js LTS release, per the policy of optimizing for current LTS. undici 7 is the transition release between the old (v6 `onError`) and new (v8 `onRequestStart`) dispatcher handler dialects, shipping both `wrap-handler` and `unwrap-handler` to translate in either direction. A v7 dispatcher is therefore usable by the `fetch` built into Node.js 22, 24, and 26 alike, so the legacy `agent`/`httpsAgent` options now use the platform `fetch` on every supported release and responses are platform `Response` instances again. Replace the strict installed-equals-platform major check with an explicit table of the platform majors each installed major can drive. An installed major with no entry falls back to requiring an exact match, so a missing or stale entry costs only the fallback path rather than correctness. The fallback is kept rather than removed: a future undici bump is expected to need it again, since a v8 dispatcher cannot be driven by the v7 `fetch` in Node.js 24. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard fell open when both version reads failed. `parseInt` returns `NaN` rather than throwing, so neither read reached the `catch`; the table lookup fell back to `[NaN]`, and `includes` matches `NaN` to `NaN` under SameValueZero. The result was `true` -- handing the dispatcher to a platform `fetch` that may reject it, rather than the always-safe installed `fetch` the comment promises. Require both majors to be integers before comparing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ky@2` buffers the error body regardless of content type, so `error.data` is now set for any error response -- an object for JSON, a string otherwise -- where v4 left it `undefined` unless the content type included `json`. Code using `if(error.data)` as a "the server sent JSON" test needs updating.
Karma is unmaintained. Vitest replaces the runner, the browser test harness, and the coverage tool with a single dependency and config. - Add `vitest.config.js` with `node` and `browser` projects. The browser project runs Chromium through `@vitest/browser-playwright`, replacing karma and karma-webpack. - Keep the existing `should`-style assertions unchanged. `tests/setup.js` installs the global `should` from vitest's re-exported chai, so `chai` is no longer a dependency. - Start the HTTP/HTTPS test servers in `tests/globalSetup.js` and pass their ephemeral hosts to tests with `inject()`. This replaces starting them in the karma config and injecting the hosts via webpack's `DefinePlugin`, and lets `tests/utils-browser.js` be removed. - Split the suite by environment into `10-client-api.spec.js` (shared), `20-node.spec.js`, and `30-browser.spec.js` instead of branching on `isNode` at runtime. This keeps node-only modules out of the browser project and drops the `detect-node` dependency. Test bodies are unchanged; both projects still run 17 tests. - Report coverage with `@vitest/coverage-v8` across both projects, so browser-only code paths are now covered. Reported totals shift slightly because vitest and c8 count executable lines differently. - Replace the `test-karma` CI job with `test-browser`, and install and cache Playwright's Chromium in the browser and coverage jobs. Add an `exports` field with a `browser` condition for `agentCompatibility` and import it via a self-reference. Vite does not apply the top-level `browser` field to package-internal relative imports, so this is also a fix for browser bundlers, which would otherwise pull `undici` into their builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage was 79.51% of statements. Add tests for the paths that had none: - `httpClient.create()` and the proxied `stop` signal. The existing "can use create()" test never called `create()`. - A direct call with a method that is not proxied, which goes straight to `ky` and so skips the response and error handling. - `convertAgent` declining to override a custom `fetch` from another lib. - The `fetch` override used when the installed undici cannot drive the platform `fetch`. No supported node takes that path with `undici@7`, so the test forces it by reporting an incompatible platform undici major. - Importing when the version read fails, which must not throw at module load. Now at 100% of statements, lines, and functions, and 98.27% of branches. The one uncovered branch is the fallback for an installed undici major that is not in the compatibility table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `Possible CORS error` message was produced by matching the literal string `Failed to fetch`, which is Chromium's wording for a failed or blocked `fetch`. Firefox reports `NetworkError when attempting to fetch resource.` and WebKit reports `Load failed`, so neither matched and both fell through to `ky`'s generic network error. Match against a set of the three engines' messages instead. Node.js `fetch failed` is deliberately excluded: there is no CORS in Node.js, so the hint would be misleading there. All three strings were measured, not assumed, and each is exercised by the browser test matrix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is what keeps the per-engine CORS messages honest, and running the shared suite in each engine surfaces any other behavior difference. Three changes were needed to make the other engines work: - Advertise the test servers as `127.0.0.1` rather than the `0.0.0.0` that `address()` reports. Only Chromium treats `0.0.0.0` as loopback when used as a request host; Firefox and WebKit refuse to connect, which failed every test that used a local server. - Replace the raw `--no-sandbox` launch args with `chromiumSandbox: false`. WebKit rejects unknown options and fails to launch; the `playwright` option is applied to Chromium only. - Use the `istanbul` coverage provider rather than `v8`. v8 coverage is gathered over CDP, which only Chromium supports, so `coverage` refused to start once the other engines were added. `istanbul` counts default parameters and some conditionals that `v8` did not, which surfaced an untested documented option, so also test `parseBody: false` and `create()`/`extend()` with no overrides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
davidlehn
force-pushed
the
update-deps
branch
from
September 29, 2026 02:45
2f9a16c to
acc3caf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Karma is unmaintained. Vitest replaces the runner, the browser test harness, and the coverage tool with a single dependency and config.
vitest.config.jswithnodeandbrowserprojects. The browser project runs Chromium through@vitest/browser-playwright, replacing karma and karma-webpack.should-style assertions unchanged.tests/setup.jsinstalls the globalshouldfrom vitest's re-exported chai, sochaiis no longer a dependency.tests/globalSetup.jsand pass their ephemeral hosts to tests withinject(). This replaces starting them in the karma config and injecting the hosts via webpack'sDefinePlugin, and letstests/utils-browser.jsbe removed.10-client-api.spec.js(shared),20-node.spec.js, and30-browser.spec.jsinstead of branching onisNodeat runtime. This keeps node-only modules out of the browser project and drops thedetect-nodedependency. Test bodies are unchanged; both projects still run 17 tests.@vitest/coverage-v8across both projects, so browser-only code paths are now covered. Reported totals shift slightly because vitest and c8 count executable lines differently.test-karmaCI job withtest-browser, and install and cache Playwright's Chromium in the browser and coverage jobs.Add an
exportsfield with abrowsercondition foragentCompatibilityand import it via a self-reference. Vite does not apply the top-levelbrowserfield to package-internal relative imports, so this is also a fix for browser bundlers, which would otherwise pullundiciinto their builds.