Skip to content

Update dev dependencies. - #51

Open
davidlehn wants to merge 23 commits into
mainfrom
update-deps
Open

Update dev dependencies.#51
davidlehn wants to merge 23 commits into
mainfrom
update-deps

Conversation

@davidlehn

Copy link
Copy Markdown
Member

No description provided.

@codecov-commenter

codecov-commenter commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.27%. Comparing base (4a92bb3) to head (8590f56).

Files with missing lines Patch % Lines
lib/httpClient.js 93.10% 2 Missing ⚠️
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     
Files with missing lines Coverage Δ
lib/agentCompatibility.js 81.74% <100.00%> (-11.54%) ⬇️
lib/index.js 100.00% <100.00%> (ø)
lib/httpClient.js 92.76% <93.10%> (-0.75%) ⬇️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a92bb3...8590f56. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@davidlehn
davidlehn requested review from applesnort and dlongley July 25, 2026 03:03
davidlehn and others added 20 commits August 11, 2026 20:01
- 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 applesnort left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Comment thread lib/httpClient.js
// 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'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread CHANGELOG.md
- Remove `push`.
- Add `query`, `options`, and `trace` to align with `ky@2`.
- **BREAKING**: Update dependencies:
- `ky@2`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread lib/agentCompatibility.js
} catch{
const compatible =
COMPATIBLE_PLATFORM_MAJORS[installedMajor] ?? [installedMajor];
return compatible.includes(platformMajor);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread CHANGELOG.md

### Changed
- **BREAKING**: Revert CJS related workarounds from v3.0.0.
- `kyOriginalPromise` no longer exported.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants