Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 94.05% 93.37% -0.69%
==========================================
Files 4 3 -1
Lines 303 347 +44
==========================================
+ Hits 285 324 +39
- Misses 18 23 +5
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.
applesnort
left a comment
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.
There was a problem hiding this comment.
Fixed. I was looking at the wrong list of methods when making that change. And query support was added, and I saw it in the readme and code, but it's not released yet.
| - 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.
There was a problem hiding this comment.
Note added to changelog. Hopefully that's enough. And hopefully package users do testing.
| } 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.
There was a problem hiding this comment.
- kyPromise issue was a typo. Fixed in the commit that introduced the issue.
- There's generic supported platform text in the readme. That change in newer packages was done in part to avoid having to update the readme all the time. The docs didn't have CJS examples, and modern platforms support ESM so I figured that was fine not being explicit.
- `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`; it was never an HTTP method. The list mirrors the helpers
`ky` implements, so add a test that it cannot drift from `ky`'s registry,
and one covering a non-proxied method via `httpClient(url, {method})`.
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.
- 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.
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.
`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.
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.
`engines` requires node >=22, so the node 18.2+ guard on agent conversion is always true.
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.
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.
`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.
- `ky@2.1`. - `undici@7.30`. - Dev: `eslint@10.11`, `mocha@12`, `webpack@5.111`.
`ky@2.1` adds `query` to its helper registry and exposes `ky.query`, so
add it to the proxied set. `httpClient.query` and `{method: 'query'}`
now go through response handling, setting `.data` and normalizing
errors like the other helpers, rather than falling through to `ky`
directly.
Add a QUERY echo route to the test server. The default `cors()` methods
omit QUERY, so the route lists it for the browser preflight.
`create()` spread the overrides into `DEFAULT_HEADERS` as a plain object before converting to `Headers`. Plain object keys are case-sensitive, so an `accept` override survived alongside the default `Accept`, and the `Headers` constructor combined them into one value. The same spread silently dropped overrides given as a `Headers` instance. Set the overrides onto a `Headers` built from the defaults instead, treating an `undefined` value as a deletion like ky does. Wrapping headers in `new Headers()` also threw on ky's `replaceOption()` marker. Recognize the marker and honor it: `extend()` replaces the parent's headers and `create()` replaces the defaults. The `create()` test never called `create()`, which hid the case bug. Make it do so, and cover `Headers` instances and `replaceOption()`.
ky wraps fetch's error in a `NetworkError`, which moves node's system error code from `err.cause` to `err.cause.cause`. The `if(cause.code)` guard then skipped the assertion on every runtime. Walk the `cause` chain for the code, and require it on node.
`ky@2` reads the error response body to fill `error.data`, so reading it again from `error.response` throws "Body has already been read".
The proxy test only checked a fixed list of names, so a helper added by a `ky` upgrade went unnoticed unless it was one of the names checked for, as `query` happened to be. Compare the proxied list with every request helper on `ky`, and with the methods `httpClient` actually has, so an addition or removal on either side fails the test. The list stays hand-maintained: a new helper is new public API and should be added deliberately.
The parameter shadowed the imported base `ky`, which `createInstance` relies on for its `parent === ky` check. Inside `_createHttpClient` the name meant the derived instance instead, e.g. `extend` passing `parent: ky`, so an edit expecting the base would quietly get the wrong one.
`ky@2` already parses `error.data` by content type, and a text body is a string with no `message`, so reading `error.data?.message` needs no content-type guard of its own. Dropping it also keeps the message logic in step with ky's JSON detection, including a custom `parseJson`. Add a test for the JSON `message` path, which had no coverage.
2f9a16c to
acc3caf
Compare
The wrapper API is deliberately all async, a holdover from keeping the CJS and ESM builds consistent that is kept to avoid breaking callers. Pin down that `stop` resolves to `ky.stop` rather than returning it directly.
Cover the `undefined` value path in `_mergeHeaders`: the header is deleted, like in ky, instead of being sent as the string 'undefined'.
Only the browser run reached the "Possible CORS error" message, since it needs a real CORS failure. Simulate the browser's bare `TypeError: Failed to fetch` with a `fetch` option so every run covers it.
`convertAgent` leaves options alone when they already carry a `fetch` it did not create, so a `fetch` supplied by another library is neither replaced nor handed a `dispatcher` built from the agent. Cover that path, checking for the `dispatcher`: on a compatible platform the custom `fetch` would still be called without the guard.
`convertAgent` caches the dispatcher (and, on an incompatible platform, the `fetch` override) per agent so connections are pooled across requests. Check the same agent gets the same conversion and a different agent does not.
`replaceOption()` headers in `create()` replace `DEFAULT_HEADERS` instead of being merged into them. Only `extend()` had a `replaceOption()` test.
applesnort
left a comment
applesnort
left a comment
There was a problem hiding this comment.
Approving. Ran test-node on 22 and 24 locally, plus a probe of the create/extend header cases, replaceOption, query, and HTML vs JSON error bodies — all behave as the changelog says. Nits: ### Usage sits under Install but the TOC lists it top-level; and the title undersells a 5.0.0 — maybe a one-line body saying so.
4 participants
No description provided.