Repository navigation
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 93.37% 100.00% +6.62%
================================================
Files 3 4 +1
Lines 347 87 -260
================================================
- Hits 324 87 -237
+ Misses 23 0 -23
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
davidlehn
force-pushed
the
update-deps
branch
from
September 29, 2026 02:45
2f9a16c to
acc3caf
Compare
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, except that the node-only `ECONNREFUSED` cause check of the connection refused test becomes its own test in `20-node.spec.js`. - 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.
Add tests for the paths that had none: - `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.48% of branches. The one uncovered branch is the fallback for an installed undici major that is not in the compatibility table.
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.
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.
Chromium can report `Failed to fetch (example.com)` and WebKit `Load failed (example.com)`. Only exact messages were matched, so these fell through to `ky`'s generic network error without the CORS hint. Accept a ` (<host>)` suffix on those two messages, following `is-network-error`, which `ky` inlines for its own network error detection. `ky`'s `isNetworkError` is not used directly because it also matches Node.js and other non-browser runtimes, where a CORS hint is misleading. Cover every engine's wording and the suffixed variants with simulated `fetch` failures, and check that Node.js's `fetch failed` gets no hint.
- Update to `vitest@5`, `@vitest/browser-playwright@5`, and `@vitest/coverage-istanbul@5`. - Update to `[email protected]`. - Ignore `.vitest`, where `vitest@5` writes attachments, instead of `.vitest-attachments`. `undici` is kept at v7 to match the undici built into the current Node.js LTS release.
Vitest reports tests over `slowTestThreshold` (300ms) as slow, and has no per-test override for that threshold. Several tests exceeded it without testing anything that needed the time. - Use a 100ms timeout against a 1s delay in the `TimeoutError` test rather than a 1s timeout against a 2s delay. - Pass `retry: 0` in the connection refused, network error, and CORS error tests. `ky` retries network errors for GET with 300ms and 600ms backoff, which added ~900ms to each, and none of them tests retry behavior.
The self-reference through the package name only resolves where the bundler supports `exports` and self-reference. Metro without package exports enabled (the default before React Native 0.79) and webpack 4 would look for a nonexistent `agentCompatibility.js` at the package root and fail the build. The relative import is remapped for them by the top-level `browser` and `react-native` fields, as before. The self-reference was added for the vitest browser project, but Vite does apply the `browser` field to this relative import: browser-project coverage shows only `agentCompatibility-browser.js` loaded. Drop the changelog entry that described it as a bundler fix.
`[email protected]` merges headers case-insensitively, deletes a header given as `undefined`, and honors `replaceOption()` on `headers`. That is everything the local `_mergeHeaders`/`_unwrapReplaceOption` helpers did, so remove them along with the `create()`/`extend()` split. Every client now extends a base instance that carries `DEFAULT_HEADERS`. This also drops the lookup of `ky`'s private `replaceOption` symbol by position, which would have broken silently if `ky` changed the wrapper's symbols.
It imported `{Agent}` but called `new https.Agent()`, and passed a
`headers` variable defined only in the previous example.
Teardown called `close()` without waiting for it, and `close()` alone leaves open keep-alive and in-flight connections (such as a timed-out `/delay` request) to run until they time out. Close every connection and wait for both servers.
Playwright's CI docs advise against caching browser binaries: restoring the cache takes about as long as downloading them, and on Linux the OS dependencies from `--with-deps` have to be installed on every run anyway. The cache was also keyed on `package.json`, which doesn't change when a new release of the `playwright` range installs new browser builds, so it restored stale browsers that were then downloaded over.
Rename `test-node`, `test-browser`, `test-watch`, and `coverage-ci` to `test:node`, `test:browser`, `test:watch`, and `coverage:ci`, the common convention for script variants. The CI job IDs keep their hyphenated names, matching other projects.
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.