Update dev dependencies. - #51
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 94.05% 88.27% -5.79%
==========================================
Files 4 3 -1
Lines 303 290 -13
==========================================
- Hits 285 256 -29
- Misses 18 34 +16
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
- Test on Node.js >=22. - Update `engines.node` to `>=22`. - Update README requirements section.
- Improve formatting. - Add more common README sections.
- `kyOriginalPromise` no longer exported. - `ky` is again exported. - Change from using `ky` promises to regular instances.
- After `ky@2` update the error body is already available in `error.data` and trying to get JSON again will fail.
- Remove `push`. - Add `query`, `options`, and `trace` to align with `ky@2`.
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>
applesnort
left a comment
There was a problem hiding this comment.
Four comments, one blocking.
The blocker: query/options/trace were added to PROXY_METHODS, but ky@2 doesn't implement them, so all three throw. {method: 'options'} worked in v4, so this is a regression rather than only a new API that doesn't work, and trace can't work in any ky version because Fetch forbids the method.
Everything here was checked by running this branch (ky@2.0.2 / undici@7.29.0) and origin/main (ky@1.14.3 / undici@6.28.0) side by side on Node 26 against a local server; the before/after output is in the inline comments.
Two smaller things: an error.data contract change that's missing from the CHANGELOG's BREAKING list, and a fail-open in the undici compatibility guard when both version reads fail.
The rest of the approach holds up — the undici@7 compatibility table is a good call, and the Headers-based default headers don't break ky's "undefined removes a header" contract on either the instance or the per-request path (I tested both, since that was my first suspicion).
| // methods to proxy from ky | ||
| const PROXY_METHODS = new Set([ | ||
| 'get', 'post', 'put', 'push', 'patch', 'head', 'delete' | ||
| 'get', 'post', 'put', 'patch', 'head', 'delete', 'query', 'options', 'trace' |
There was a problem hiding this comment.
query, options, and trace aren't on ky@2, so adding them here makes all three throw — and for two of them this is a regression on a path that worked in v4, not just a new API that doesn't.
ky@2.0.2 exposes only get, post, put, patch, head, delete. Reflect.ownKeys(ky) gives length, name, get, post, put, patch, head, delete, create, extend, stop, retry. So ky[method] is undefined for the three new entries and the generated wrapper dies on .apply:
httpClient.query() -> TypeError: Cannot read properties of undefined (reading 'apply')
httpClient.options() -> TypeError: Cannot read properties of undefined (reading 'apply')
httpClient.trace() -> TypeError: Cannot read properties of undefined (reading 'apply')
The regression is in the direct-call dispatch at lines 57-59. Because these names are now in PROXY_METHODS, httpClient(url, {method}) is diverted into the broken proxy branch instead of falling through to ky's generic path, where options previously worked:
v4 httpClient(url, {method: 'options'}) -> OK
v5 httpClient(url, {method: 'options'}) -> TypeError
v4 httpClient(url, {method: 'query'}) -> HTTPError 400 (request reached the server)
v5 httpClient(url, {method: 'query'}) -> TypeError
trace is a separate problem: it can't be supported by any ky version, because Fetch forbids the method. v4 already reports this, and the error comes from the platform, not from ky:
v4 httpClient(url, {method: 'trace'}) -> TypeError: 'trace' HTTP method is unsupported.
Nothing in the new tests/10-client-api.spec.js calls any of the three, which is why CI is green. The CHANGELOG lists them as a BREAKING addition ("Add query, options, and trace to align with ky@2"), so as it stands v5 ships three documented methods that throw on first call, and withdrawing them later costs another major.
Suggested fix: drop trace outright, and for query/options either remove them from PROXY_METHODS and the CHANGELOG entry, or keep them and route through ky(url, {...options, method}) rather than ky[method]. Either way options needs to keep working via {method: 'options'}, and a test per proxied method would stop the set drifting from ky's registry on the next bump — deriving it from the instance (typeof ky[m] === 'function') would make drift impossible.
Removing push is right regardless — it was never an HTTP method.
| - Remove `push`. | ||
| - Add `query`, `options`, and `trace` to align with `ky@2`. | ||
| - **BREAKING**: Update dependencies: | ||
| - `ky@2`. |
There was a problem hiding this comment.
error.data semantics changed and it isn't in the BREAKING list. Under v4 the error body was read only when the response content type included json; ky@2 buffers the error body regardless of content type, so .data is now populated for non-JSON error responses, as a string:
v4 text/plain 500 -> error.data = undefined
v5 text/plain 500 -> error.data = "boom-plaintext"
That matters because if(err.data) was a serviceable "the server sent a JSON error" test, and after v5 an HTML 502 from a reverse proxy makes it truthy — so consumer branches that assumed an object will now take the JSON path and read err.data.message off a string. Worth an explicit BREAKING entry describing the new .data contract (populated for any content type; object for JSON, string otherwise).
For the record, one thing that looks like a regression here isn't: await error.response.json() in a consumer's catch throws TypeError: Body is unusable on both v4 and v5, because v4's _handleError already consumed the body itself. No need to chase that one.
| } catch{ | ||
| const compatible = | ||
| COMPATIBLE_PLATFORM_MAJORS[installedMajor] ?? [installedMajor]; | ||
| return compatible.includes(platformMajor); |
There was a problem hiding this comment.
The guard fails open when both version reads fail, which is the one case the comment above it promises is safe. parseInt returns NaN rather than throwing, so neither read reaches the catch, and [NaN].includes(NaN) is true under SameValueZero. Evaluating the block exactly as written:
both readable (normal) compatible=[6,7,8] -> platformFetchCompatible = true
versions.undici absent compatible=[6,7,8] -> platformFetchCompatible = false
undici pkg version absent compatible=[NaN] -> platformFetchCompatible = false
BOTH absent compatible=[NaN] -> platformFetchCompatible = true <-- fails open
Three of the four cases are right. In the fourth, COMPATIBLE_PLATFORM_MAJORS[NaN] is undefined, so ?? [installedMajor] yields [NaN], and the includes compares NaN to NaN and matches — so the dispatcher gets handed to a platform fetch that may reject it with "invalid onError method", instead of taking the always-safe installed-fetch path the comment describes ("Either way fall back to false").
Both reads failing at once is unlikely, so this is low severity — but the fix is small: require Number.isInteger(installedMajor) && Number.isInteger(platformMajor) before comparing, or return false early when either parse is NaN.
|
|
||
| ### Changed | ||
| - **BREAKING**: Revert CJS related workarounds from v3.0.0. | ||
| - `kyOriginalPromise` no longer exported. |
There was a problem hiding this comment.
Minor, and only about wording: this entry names kyOriginalPromise, but the symbol consumers actually imported was kyPromise (lib/index.js had export {kyOriginalPromise as kyPromise}). Anyone grepping the changelog for the name in their own import list won't find it — worth naming both.
FWIW I checked 15 DB repos that depend on this package: none reference either name, and none require() the package, so the CJS removal and this rename both look free in practice.
Separately, the new README Install section doesn't state the Node floor (>=22) or that CJS support is gone. Both are in the CHANGELOG, but the README is what people read first.
No description provided.