Skip to content

feat: swapping our node-fetch for unidici / native #1514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
orta merged 1 commit into from
Jun 25, 2026
Merged

feat: swapping our node-fetch for unidici / native #1514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@
"@types/lodash.memoize": "^4.1.3",
"@types/micromatch": "^3.1.0",
"@types/node": "18.19.18",
"@types/node-fetch": "^2.5.12",
"@types/parse-github-url": "^1.0.3",
"@types/prettier": "^1.16.1",
"@types/readline-sync": "^1.4.3",
Expand All @@ -132,7 +131,7 @@
"jest-json-reporter": "^1.2.2",
"lint-staged": "^12.3.7",
"madge": "^6.0.1",
"nock": "^13.2.0",
"nock": "^14.0.15",
"pkg": "^5.8.1",
"prettier": "^2.5.1",
"release-it": "^18.1.2",
Expand All @@ -152,8 +151,6 @@
"core-js": "^3.8.2",
"debug": "^4.1.1",
"fast-json-patch": "^3.0.0-1",
"http-proxy-agent": "^7.0.2",
"https-proxy-agent": "^7.0.2",
"hyperlinker": "^1.0.0",
"ini": "^5.0.0",
"json5": "^2.2.3",
Expand All @@ -166,7 +163,6 @@
"memfs-or-file-map-to-github-branch": "^1.3.0",
"micromatch": "^4.0.4",
"node-cleanup": "^2.1.2",
"node-fetch": "^2.6.7",
"override-require": "^1.1.1",
"parse-diff": "^0.7.0",
"parse-github-url": "^1.0.2",
Expand All @@ -176,7 +172,8 @@
"readline-sync": "^1.4.9",
"regenerator-runtime": "^0.13.9",
"require-from-string": "^2.0.2",
"supports-hyperlinks": "^4.3.0"
"supports-hyperlinks": "^4.3.0",
"undici": "6.21.1"

@fbartho fbartho Jun 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://www.npmjs.com/package/undici This seems to suggest that the version you selected is several major versions behind. (unless I’m confused?).

I’m not super familiar with undici, but if we’re swapping out dependencies, I would have expected to be on the latest version.

@nicholasgriffintn nicholasgriffintn Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should have wrote the reason for that, I forgot to. I'll update the description.

Unidici v8 drops support for node 20 and v7 dropped support for node 18.

It would be best to be on the latest version but that would require Danger JS to presumably also drop support for 18 and 20.

@fbartho fbartho Jun 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Huh! Not my decision to make, but this might be enough for me to suggest moving to node v24 as our minimum?

@nicholasgriffintn nicholasgriffintn Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah happy to update it, just working from the current support that's documented in the package.

@orta orta Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is worth keeping lower, backwards compat is pretty important to this project 👍🏻

@glensc glensc Jul 22, 2026

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.

but it shouldn't be pinned to certain version, but use semver range: "undici": "^6.21.1"

@nicholasgriffintn nicholasgriffintn Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Personally, I always use pinned to avoid supply chain attacks and then allow automations to upgrade via PRs like dependabot.

@glensc glensc Jul 22, 2026

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.

You shouldn't pin in libraries, as then downstream apps can't upgrade, but rather require library to update pin again.

However, this concrete dependency was already upgraded due same reason, that the pinned version needs security update: #1518, but it has no effect downstream, until another danger-js npm release is made.

@nicholasgriffintn nicholasgriffintn Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not strictly true, sure thats a usability concern, but doing so is causing security concerns its just which one you see as more of a problem.

but sure, seems to have been changed, my default is always to pin nowadays, outside of peer deps.

@glensc glensc Jul 23, 2026

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.

pinning in libraries moves burden of updating dependency to library maintainer. if library uses version range, you can update dependency in your app.

surely, certain package mangers have "overrides" to overcome pinned version in library, but that goes more to danger zone, as there's no validation what version you pick. could be totally incompatible version.

},
"resolutions": {
"before-after-hook": "2.2.0",
Expand Down
84 changes: 25 additions & 59 deletions source/api/_tests/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import * as http from "http"
import * as node_fetch from "node-fetch"

import type { HttpProxyAgent } from "http-proxy-agent"
import type { HttpsProxyAgent } from "https-proxy-agent"
import { ProxyAgent } from "undici"
import { api } from "../fetch"

interface ResponseMock {
Expand Down Expand Up @@ -42,48 +40,16 @@ class TestServer {
}
}

class TestProxy {
isRunning = false
private port = 30002
private hostname = "localhost"
private router = (_req: any, res: any) => {
res.statusCode = 200
res.end(null)
}
private server = http.createServer(this.router)

start = async (): Promise<void> => {
return new Promise<void>((resolve, reject) => {
this.isRunning = true
this.server.on("error", (e) => {
reject(e)
})
this.server.listen(this.port, this.hostname, undefined, () => resolve())
})
}
stop = async (): Promise<void> => {
return new Promise<void>((resolve, reject) => {
this.isRunning = false
this.server.close((err: any) => (err ? reject(err) : resolve()))
})
}
}

describe("fetch", () => {
let url: string
let server = new TestServer()
let proxy = new TestProxy()

beforeEach(() => {
url = "http://localhost:30001/"
})

afterEach(async () => {
await server.stop()

if (proxy.isRunning) {
await proxy.stop()
}
})

it("handles json success", async () => {
Expand Down Expand Up @@ -125,51 +91,51 @@ describe("fetch", () => {
expect(await response.text()).toBe(body)
})

it("sets proxy agent when HTTPS_PROXY env variable is defined", async () => {
it("sets proxy dispatcher when HTTPS_PROXY env variable is defined", async () => {
const proxyUrl = "http://localhost:30002/"
const abortController = new AbortController()
abortController.abort()

await proxy.start()
await server.start({})

let options: node_fetch.RequestInit = { agent: undefined }
await api(url, options, true, { HTTPS_PROXY: proxyUrl })
let agent = options.agent as HttpsProxyAgent<string>
expect(agent["proxy"].href).toBe(proxyUrl)
let options = { method: "GET", dispatcher: undefined, signal: abortController.signal }
await api(url, options, true, { HTTPS_PROXY: proxyUrl }).catch(() => undefined)
expect(options.dispatcher).toBeInstanceOf(ProxyAgent)
})

it("sets proxy agent when https_proxy env variable is defined", async () => {
it("sets proxy dispatcher when https_proxy env variable is defined", async () => {
const proxyUrl = "http://localhost:30002/"
const abortController = new AbortController()
abortController.abort()

await proxy.start()
await server.start({})

let options: node_fetch.RequestInit = { agent: undefined }
await api(url, options, true, { https_proxy: proxyUrl })
let agent = options.agent as HttpsProxyAgent<string>
expect(agent["proxy"].href).toBe(proxyUrl)
let options = { method: "GET", dispatcher: undefined, signal: abortController.signal }
await api(url, options, true, { https_proxy: proxyUrl }).catch(() => undefined)
expect(options.dispatcher).toBeInstanceOf(ProxyAgent)
})

it("sets proxy agent when HTTP_PROXY env variable is defined", async () => {
it("sets proxy dispatcher when HTTP_PROXY env variable is defined", async () => {
const proxyUrl = "http://localhost:30002/"
const abortController = new AbortController()
abortController.abort()

await proxy.start()
await server.start({})

let options: node_fetch.RequestInit = { agent: undefined }
await api(url, options, true, { HTTP_PROXY: proxyUrl })
let agent = options.agent as HttpProxyAgent<string>
expect(agent["proxy"].href).toBe(proxyUrl)
let options = { method: "GET", dispatcher: undefined, signal: abortController.signal }
await api(url, options, true, { HTTP_PROXY: proxyUrl }).catch(() => undefined)
expect(options.dispatcher).toBeInstanceOf(ProxyAgent)
})

it("sets proxy agent when http_proxy env variable is defined", async () => {
it("sets proxy dispatcher when http_proxy env variable is defined", async () => {
const proxyUrl = "http://localhost:30002/"
const abortController = new AbortController()
abortController.abort()

await proxy.start()
await server.start({})

let options: node_fetch.RequestInit = { agent: undefined }
await api(url, options, true, { http_proxy: proxyUrl })
let agent = options.agent as HttpProxyAgent<string>
expect(agent["proxy"].href).toBe(proxyUrl)
let options = { method: "GET", dispatcher: undefined, signal: abortController.signal }
await api(url, options, true, { http_proxy: proxyUrl }).catch(() => undefined)
expect(options.dispatcher).toBeInstanceOf(ProxyAgent)
})
})
60 changes: 27 additions & 33 deletions source/api/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
import { debug } from "../debug"
import * as node_fetch from "node-fetch"
import { fetch as undiciFetch, Headers, ProxyAgent } from "undici"
import type { Request, RequestInit, Response } from "undici"
import AsyncRetry from "async-retry"

import { HttpProxyAgent } from "http-proxy-agent"
import { HttpsProxyAgent } from "https-proxy-agent"
import { debug } from "../debug"

import AsyncRetry from "async-retry"
export type FetchResponse = Omit<Response, "json"> & {
json(): Promise<any>
}

const d = debug("networking")
declare const global: any

const isJest = typeof jest !== "undefined"
const warn = isJest ? () => "" : console.warn

const shouldRetryRequest = (res: node_fetch.Response) => {
const shouldRetryRequest = (res: FetchResponse) => {
// Don't retry 4xx errors other than 401. All 4xx errors can probably be ignored once
// the Github API issue causing https://github.com/danger/peril/issues/440 is fixed
return res.status === 401 || (res.status >= 500 && res.status <= 599)
Expand All @@ -25,15 +27,11 @@ const shouldRetryRequest = (res: node_fetch.Response) => {
* @param {fetch.RequestInit} [init] the usual options
* @returns {Promise<fetch.Response>} network-y promise
*/
export async function retryableFetch(
url: string | node_fetch.Request,
init: node_fetch.RequestInit
): Promise<node_fetch.Response> {
export async function retryableFetch(url: string | Request, init: RequestInit): Promise<FetchResponse> {
const retries = isJest ? 1 : 3
return AsyncRetry(
async (_, attempt) => {
const originalFetch = node_fetch.default
const res = await originalFetch(url, init)
const res = await undiciFetch(url, init)

// Throwing an error will trigger a retry
if (attempt <= retries && shouldRetryRequest(res)) {
Expand All @@ -60,14 +58,15 @@ export async function retryableFetch(
* @returns {Promise<fetch.Response>} network-y promise
*/
export function api(
url: string | node_fetch.Request,
init: node_fetch.RequestInit,
url: string | Request,
init: RequestInit,
suppressErrorReporting?: boolean,
processEnv: NodeJS.ProcessEnv = process.env
): Promise<node_fetch.Response> {
): Promise<FetchResponse> {
const isTests = typeof jest !== "undefined"
if (isTests && !url.toString().includes("localhost")) {
const message = `No API calls in tests please: ${url}`
const requestUrl = typeof url === "string" ? url : url.url
if (isTests && !requestUrl.includes("localhost")) {
const message = `No API calls in tests please: ${requestUrl}`
debugger
throw new Error(message)
}
Expand All @@ -82,18 +81,14 @@ export function api(
const showToken = processEnv["DANGER_VERBOSE_SHOW_TOKEN"]
const token = processEnv["DANGER_GITHUB_API_TOKEN"] || processEnv["GITHUB_TOKEN"]

if (init.headers) {
for (const prop in init.headers) {
if (init.headers.hasOwnProperty(prop)) {
// Don't show the token for normal verbose usage
if (init.headers[prop].includes(token) && !showToken) {
output.push("-H", `"${prop}: [API TOKEN]"`)
continue
}
output.push("-H", `"${prop}: ${init.headers[prop]}"`)
}
new Headers(init.headers).forEach((value, prop) => {
// Don't show the token for normal verbose usage
if (token && value.includes(token) && !showToken) {
output.push("-H", `"${prop}: [API TOKEN]"`)
return
}
}
output.push("-H", `"${prop}: ${value}"`)
})

if (init.method === "POST") {
// const body:string = init.body
Expand All @@ -107,16 +102,15 @@ export function api(
d(output.join(" "))
}

let agent = init.agent
let dispatcher = init.dispatcher
const proxy =
processEnv["HTTPS_PROXY"] || processEnv["https_proxy"] || processEnv["HTTP_PROXY"] || processEnv["http_proxy"]

if (!agent && proxy) {
let secure = url.toString().startsWith("https")
init.agent = secure ? new HttpsProxyAgent(proxy) : new HttpProxyAgent(proxy)
if (!dispatcher && proxy) {
init.dispatcher = new ProxyAgent(proxy)
}

return retryableFetch(url, init).then(async (response: node_fetch.Response) => {
return retryableFetch(url, init).then(async (response: FetchResponse) => {
// Handle failing errors
if (!suppressErrorReporting && !response.ok) {
// we should not modify the response when an error occur to allow body stream to be read again if needed
Expand Down
24 changes: 6 additions & 18 deletions source/platforms/bitbucket_cloud/BitBucketCloudAPI.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import { debug } from "../../debug"
import * as node_fetch from "node-fetch"
import { Agent } from "http"
import { HttpsProxyAgent } from "https-proxy-agent"
import { URLSearchParams } from "url"

import { Env } from "../../ci_source/ci_source"
import { dangerIDToString } from "../../runner/templates/bitbucketCloudTemplate"
import { api as fetch } from "../../api/fetch"
import type { FetchResponse } from "../../api/fetch"
import {
BitBucketCloudPagedResponse,
BitBucketCloudPRDSL,
Expand Down Expand Up @@ -385,15 +383,6 @@ export class BitBucketCloudAPI implements BitBucketCloudAPIDSL {

private performAPI = (url: string, headers: any = {}, body: any = {}, method: string, suppressErrors?: boolean) => {
this.d(`${method} ${url}`)
// Allow using a proxy configured through environmental variables
// Remember that to avoid the error "Error: self signed certificate in certificate chain"
// you should also do: "export NODE_TLS_REJECT_UNAUTHORIZED=0". See: https://github.com/request/request/issues/2061
let agent: Agent | undefined = undefined
let proxy = process.env.http_proxy || process.env.https_proxy
if (proxy) {
agent = new HttpsProxyAgent(proxy)
}

return this.fetch(
url,
{
Expand All @@ -403,26 +392,25 @@ export class BitBucketCloudAPI implements BitBucketCloudAPIDSL {
"Content-Type": "application/json",
...headers,
},
agent,
},
suppressErrors
)
}

get = (url: string, headers: any = {}, suppressErrors?: boolean): Promise<node_fetch.Response> =>
get = (url: string, headers: any = {}, suppressErrors?: boolean): Promise<FetchResponse> =>
this.api(url, headers, null, "GET", suppressErrors)

post = (url: string, headers: any = {}, body: any = {}, suppressErrors?: boolean): Promise<node_fetch.Response> =>
post = (url: string, headers: any = {}, body: any = {}, suppressErrors?: boolean): Promise<FetchResponse> =>
this.api(url, headers, JSON.stringify(body), "POST", suppressErrors)

put = (url: string, headers: any = {}, body: any = {}): Promise<node_fetch.Response> =>
put = (url: string, headers: any = {}, body: any = {}): Promise<FetchResponse> =>
this.api(url, headers, JSON.stringify(body), "PUT")

delete = (url: string, headers: any = {}, body: any = {}): Promise<node_fetch.Response> =>
delete = (url: string, headers: any = {}, body: any = {}): Promise<FetchResponse> =>
this.api(url, headers, JSON.stringify(body), "DELETE")
}

function throwIfNotOk(res: node_fetch.Response) {
function throwIfNotOk(res: FetchResponse) {
if (!res.ok) {
let message = `${res.status} - ${res.statusText}`
if (res.status >= 400 && res.status < 500) {
Expand Down
Loading
Loading