fix: drop deprecated node-fetch package #4503
Conversation
Signed-off-by: Gordon Smith <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request removes the deprecated node-fetch (v3.3.2) and abort-controller (v3.0.0) packages from the @hpcc-js/comms package, updating the codebase to rely on native Node.js >= v18 implementations of fetch and AbortController. The change simplifies the dependency tree and modernizes the package for current Node.js versions.
Key changes:
- Removed polyfill-based implementations in favor of native Node.js APIs
- Added comprehensive test coverage for AbortController functionality
- Cleaned up package-lock.json to remove deprecated dependencies and their transitive dependencies
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
packages/comms/tests/abortController.spec.ts |
New test file adding comprehensive coverage for AbortController functionality with various scenarios (immediate abort, delayed abort, multiple requests, GET/POST methods) |
packages/comms/src/index.node.ts |
Removed conditional polyfill imports for node-fetch and abort-controller, now assumes native Node.js >= v18 APIs are available |
packages/comms/package.json |
Removed node-fetch and abort-controller from dependencies |
package-lock.json |
Cleaned up dependency tree removing node-fetch, abort-controller, and their transitive dependencies (fetch-blob, formdata-polyfill, node-domexception, web-streams-polyfill, and various whatwg-url/webidl-conversions versions) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NodeJS >= v18 has native fetch --- | ||
| root.fetch.__defaultAgent = new Agent(); | ||
| root.fetch.__rejectUnauthorizedAgent = new Agent({ | ||
| connect: { | ||
| rejectUnauthorized: false | ||
| }); | ||
| } else { | ||
| // NodeJS >= v18 --- | ||
| root.fetch.__defaultAgent = new Agent(); | ||
| root.fetch.__rejectUnauthorizedAgent = new Agent({ | ||
| connect: { | ||
| rejectUnauthorized: false | ||
| } | ||
| }); | ||
| root.fetch.__setGlobalDispatcher = setGlobalDispatcher; | ||
| } | ||
|
|
||
| // AbortController polyfill --- | ||
| import AbortController from "abort-controller"; | ||
| if (typeof root.AbortController === "undefined") { | ||
| root.AbortController = AbortController; | ||
| } | ||
| } | ||
| }); | ||
| root.fetch.__setGlobalDispatcher = setGlobalDispatcher; |
There was a problem hiding this comment.
This code now assumes that root.fetch exists and directly accesses properties on it. Since native fetch was only added in Node.js v18, this will throw a runtime error if used with older Node.js versions. While the removal of polyfills is the intent of this PR, consider adding a runtime check and a helpful error message to guide users who might still be on older Node.js versions. For example:
if (typeof root.fetch === "undefined") {
throw new Error("@hpcc-js/comms requires Node.js >= 18.0.0 for native fetch support");
}| import { describe, it, expect } from "vitest"; | ||
|
|
||
| import { Connection, WorkunitsService } from "@hpcc-js/comms"; | ||
| import { ESP_URL, isCI } from "./testLib.ts"; |
There was a problem hiding this comment.
The imported isCI variable is never used in this test file. Consider removing this unused import to keep the code clean.
| import { ESP_URL, isCI } from "./testLib.ts"; | |
| import { ESP_URL } from "./testLib.ts"; |
1 participant
Checklist:
Testing: