fix(node): bind Dispatcher methods so Agent survives undici.compose()#155
Conversation
undici's `Dispatcher.compose()` returns a `Proxy` whose `get` trap forwards lookups to the wrapped dispatcher, so `composed.destroy()` ends up invoking `Agent#destroy` with `this === proxy`. The proxy doesn't carry the class's private-field brand, and the first `this.#destroyed = true` throws: TypeError: Cannot write private member #a to an object whose class did not declare it Bind `dispatch`, `close`, and `destroy` in the constructor so they re-route to the real Agent regardless of how they're invoked. Reported by a downstream integration (@milaboratories/pl-client) that calls `agent.compose(interceptors.retry()).destroy()` on a normal lifecycle path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review changes with Changed Files
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the mise.lock file and modifies the Node.js package to support undici.compose() by binding lifecycle methods in the Agent constructor, supported by new edge-case tests. Project requirements are also updated, raising the minimum Node.js version to 22.12.0 and adding Undici ^8.0.0. Feedback suggests using Object.assign for method binding to avoid TypeScript errors and notes that dropping Node.js 20 support is a breaking change that should be clearly documented.
There was a problem hiding this comment.
Code Review
This pull request improves compatibility with undici.compose() by binding the dispatch, close, and destroy methods in the Agent constructor and adds corresponding lifecycle tests. It also updates the mise.lock file and modifies the project requirements in the README. Feedback was provided regarding the Node.js version update, which drops support for Node.js 20; this is a breaking change that may be unintentional and should be reviewed.
Greptile SummaryThis PR fixes a
Confidence Score: 4/5The core fix is safe to merge; the only items worth a second look are the undocumented Node.js 20 support drop in the README and the absence of a proxy-routed dispatch test. The agent.ts change is minimal and correctly addresses the private-field brand-check failure. The new tests cover the documented failure scenario. The README quietly removes Node.js 20 from the supported range, which is a breaking change for consumers on that LTS line and is not mentioned anywhere in the PR description or title. packages/node/README.md — the Node.js 20 requirement drop should be confirmed as intentional and communicated to users. Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/node/README.md:153-154
**Undocumented breaking change: Node.js 20 support silently dropped**
The requirements block now lists only `>=22.12.0`, removing the previously supported `^20.19.0` range. This is a semver-breaking change for any consumer still on Node.js 20 LTS and is not mentioned in the PR title, description, or a changelog entry. If this is intentional (e.g. undici `^8.0.0` requires Node 22), it should be called out explicitly so downstream users know to expect the bump.
### Issue 2 of 2
packages/node/tests/vitest/agent-edge-cases.test.ts:250-263
**`dispatch` binding not exercised through the proxy**
The constructor now also binds `dispatch`, which accesses `this.#destroyed`, `this.#closed`, `this.#pending`, and `this.#maxBufferedRequestBodyBytes` — all private fields that would throw the same brand-check error if called with `this === proxy`. The two new tests only exercise `close` and `destroy` through the composed proxy; a test that sends a real request via `composed.dispatch(…)` (or the higher-level `composed.request(…)`) would give the same regression coverage for the `dispatch` binding.
Reviews (1): Last reviewed commit: "fix(node): bind Dispatcher methods so Ag..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ADME requirements - Add regression test exercising a full request via `composed.dispatch()` through the undici compose Proxy (greptile P2). undici's `compose()` already short-circuits `dispatch` to the closure, so the brand check can't bite there today — but the constructor binds `dispatch` defensively and this test guards against future undici trap changes. - Drop the README Requirements section. Source of truth is `package.json` (engines, peerDependencies). Keeps the README from drifting on every undici/Node bump and resolves the gemini/greptile "undocumented breaking change" feedback by removing the duplicated docs entirely. The Gemini suggestion to use `Object.assign` to bind the methods is a false positive — `tsc --noEmit` is clean with the direct assignments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
undici's
Dispatcher.compose()returns aProxywhosegettrap forwards lookups to the wrapped dispatcher, socomposed.destroy()ends up invokingAgent#destroywiththis === proxy. The proxy doesn't carry the class's private-field brand, and the firstthis.#destroyed = truethrows:TypeError: Cannot write private member #a to an object whose class
did not declare it
Bind
dispatch,close, anddestroyin the constructor so they re-route to the real Agent regardless of how they're invoked.Reported by a downstream integration (@milaboratories/pl-client) that calls
agent.compose(interceptors.retry()).destroy()on a normal lifecycle path.