klaemo commented on code in PR #383:
URL: https://github.com/apache/couchdb-nano/pull/383#discussion_r4444645168
##########
.github/workflows/ci.yaml:
##########
@@ -22,7 +22,7 @@ jobs:
strategy:
matrix:
- node-version: [20.x, 22.x, 24.x]
+ node-version: [22.x, 24.x, 26.x]
Review Comment:
Dropping Node 20 from CI seems fine to me since it's EOL, though `engines`
still says `>=20`.
##########
README.md:
##########
@@ -229,19 +229,14 @@ const agentOptions = {
connections: null,
maxRedirections: 0
}
-const undici = require('undici')
-const undiciOptions = new undici.Agent(agentOptions)
-const nano = Nano({ url: 'http://127.0.0.1:5984', undiciOptions })
+const { Agent, install } = require('undici')
+// use the installed Undici's fetch to avoid incompatibility with Node.js
built-in
+install()
Review Comment:
I'd avoid `install()` as a recommendation here. It replaces the global
`fetch`, `FormData`, `WebSocket` etc. for the whole app (and with undici 8
enables HTTP/2 by default for all of them), just to configure nano's pool.
undici 8 ships `Dispatcher1Wrapper` as the bridge for built-in fetch, and it
works without touching globals. I tested `Nano({ url, agentOptions: new
Dispatcher1Wrapper(new Agent(opts)) })` on Node 22.23 (bundled undici 6.29),
24.22 (7.30) and 26.13 (8.12): all OK. A plain undici 8 `Agent` fails on 22/24
with `invalid onRequestStart method`.
##########
test/nano.agent.test.js:
##########
@@ -5,11 +5,11 @@ const Nano = require('../lib/nano')
const undici = require('undici')
test('should be able to supply a custom agent parameters', async () => {
- const agentOptions = {
+ const opts = {
bodyTimeout: 10000
}
- const undiciOptions = new undici.Agent(agentOptions)
- const nano = Nano({ url: COUCH_URL, undiciOptions })
+ const agentOptions = new undici.Agent(opts)
+ const nano = Nano({ url: COUCH_URL, agentOptions })
assert.equal(nano.config.agent, nano.config.agent)
Review Comment:
This compares `nano.config.agent` with itself, so it passes whatever we pass
in. That's how the `undiciOptions` mistake got through. Checking against the
agent we passed would catch it:
```suggestion
assert.equal(nano.config.agent, agentOptions)
```
##########
README.md:
##########
@@ -213,7 +213,7 @@ const db = couch.use('foo');
To specify the number of connections, timeouts and pool size, supply an
`agentOptions` object when starting up Nano.
Review Comment:
This still says to pass an options object, but nano hands `agentOptions`
straight to `fetch` as the `dispatcher`. Maybe:
```suggestion
To specify the number of connections, timeouts and pool size, supply an
undici `Agent` as `agentOptions` when starting up Nano. Nano passes
`agentOptions` to _fetch_ as its `dispatcher`, so it must be a dispatcher
instance, not a plain object of options.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
