Merge pull request #35468 from overleaf/ar-fetch-utils-type-improvements

[fetch-utils] type improvements

GitOrigin-RevId: 4652054c715c4ddb19bac1f46cdf1fc3916c533d

libraries/fetch-utils/index.ts

@@ -1,8 +1,7 @@
-import _ from 'lodash'
 import { Readable, Duplex } from 'node:stream'
 import OError from '@overleaf/o-error'
-import fetch from 'node-fetch'
-import type { Response } from 'node-fetch'
+import fetch, { Headers } from 'node-fetch'
+import type { RequestInit, Response } from 'node-fetch'
 import http from 'node:http'
 import https from 'node:https'
 import net from 'node:net'
@@ -10,6 +9,23 @@ import tls from 'node:tls'
 
 let logger: { warn: (...args: any[]) => void } | undefined
 
+type BasicAuthOptions = {
+  user: string
+  password: string
+}
+
+type FetchRequestOptions = RequestInit & {
+  json?: unknown
+  basicAuth?: BasicAuthOptions
+}
+
+type ParsedFetchOptions = Omit<
+  FetchRequestOptions,
+  'json' | 'signal' | 'basicAuth'
+> & {
+  signal?: AbortSignal
+}
+
 function setLogger(loggerInstance: { warn: (...args: any[]) => void }) {
   logger = loggerInstance
 }
@@ -19,15 +35,22 @@ function setLogger(loggerInstance: { warn: (...args: any[]) => void }) {
  *
  * @throws {RequestFailedError} if the response has a failure status code
  */
-async function fetchJson(url: string | URL, opts: any = {}) {
+async function fetchJson(url: string | URL, opts: FetchRequestOptions = {}) {
   const { json } = await fetchJsonWithResponse(url, opts)
   return json
 }
 
-async function fetchJsonWithResponse(url: string | URL, opts: any = {}) {
+async function fetchJsonWithResponse(
+  url: string | URL,
+  opts: FetchRequestOptions = {}
+) {
   const { fetchOpts, detachSignal } = parseOpts(opts, url)
-  fetchOpts.headers = fetchOpts.headers ?? {}
-  fetchOpts.headers.Accept = fetchOpts.headers.Accept ?? 'application/json'
+  if (!(fetchOpts.headers instanceof Headers)) {
+    fetchOpts.headers = new Headers(fetchOpts.headers)
+  }
+  if (!fetchOpts.headers.has('Accept')) {
+    fetchOpts.headers.set('Accept', 'application/json')
+  }
 
   const response = await performRequest(url, fetchOpts, detachSignal)
   if (!response.ok) {
@@ -46,12 +69,15 @@ async function fetchJsonWithResponse(url: string | URL, opts: any = {}) {
  *
  * @throws {RequestFailedError} if the response has a failure status code
  */
-async function fetchStream(url: string | URL, opts: any = {}) {
+async function fetchStream(url: string | URL, opts: FetchRequestOptions = {}) {
   const { stream } = await fetchStreamWithResponse(url, opts)
   return stream
 }
 
-async function fetchStreamWithResponse(url: string | URL, opts: any = {}) {
+async function fetchStreamWithResponse(
+  url: string | URL,
+  opts: FetchRequestOptions = {}
+) {
   const { fetchOpts, abortController, detachSignal } = parseOpts(opts, url)
   const response = await performRequest(url, fetchOpts, detachSignal)
 
@@ -71,7 +97,7 @@ async function fetchStreamWithResponse(url: string | URL, opts: any = {}) {
  *
  * @throws {RequestFailedError} if the response has a failure status code
  */
-async function fetchNothing(url: string | URL, opts: any = {}) {
+async function fetchNothing(url: string | URL, opts: FetchRequestOptions = {}) {
   const { fetchOpts, detachSignal } = parseOpts(opts, url)
   const response = await performRequest(url, fetchOpts, detachSignal)
   if (!response.ok) {
@@ -87,7 +113,10 @@ async function fetchNothing(url: string | URL, opts: any = {}) {
  *
  * @throws {RequestFailedError} if the response has a non redirect status code or missing Location header
  */
-async function fetchRedirect(url: string | URL, opts: any = {}) {
+async function fetchRedirect(
+  url: string | URL,
+  opts: FetchRequestOptions = {}
+) {
   const { location } = await fetchRedirectWithResponse(url, opts)
   return location
 }
@@ -97,7 +126,10 @@ async function fetchRedirect(url: string | URL, opts: any = {}) {
  *
  * @throws {RequestFailedError} if the response has a non redirect status code or missing Location header
  */
-async function fetchRedirectWithResponse(url: string | URL, opts: any = {}) {
+async function fetchRedirectWithResponse(
+  url: string | URL,
+  opts: FetchRequestOptions = {}
+) {
   const { fetchOpts, detachSignal } = parseOpts(opts, url)
   fetchOpts.redirect = 'manual'
   const response = await performRequest(url, fetchOpts, detachSignal)
@@ -123,12 +155,15 @@ async function fetchRedirectWithResponse(url: string | URL, opts: any = {}) {
  *
  * @throws {RequestFailedError} if the response has a failure status code
  */
-async function fetchString(url: string | URL, opts: any = {}) {
+async function fetchString(url: string | URL, opts: FetchRequestOptions = {}) {
   const { body } = await fetchStringWithResponse(url, opts)
   return body
 }
 
-async function fetchStringWithResponse(url: string | URL, opts: any = {}) {
+async function fetchStringWithResponse(
+  url: string | URL,
+  opts: FetchRequestOptions = {}
+) {
   const { fetchOpts, detachSignal } = parseOpts(opts, url)
   const response = await performRequest(url, fetchOpts, detachSignal)
   if (!response.ok) {
@@ -145,7 +180,7 @@ class RequestFailedError extends OError {
 
   constructor(
     url: string | URL,
-    opts: any,
+    opts: Pick<FetchRequestOptions, 'method'>,
     response: Response,
     body: string | null
   ) {
@@ -163,20 +198,21 @@ class RequestFailedError extends OError {
   }
 }
 
-function setupDefaultAgent(fetchOps: any) {
+function setupDefaultAgent(fetchOpts: ParsedFetchOptions) {
   // Provide a function to get the agent for each request as there may be
   // multiple requests with different protocols due to redirects.
-  fetchOps.agent = (url: URL) =>
+  fetchOpts.agent = (url: URL) =>
     url.protocol === 'https:' ? httpsAgent : httpAgent
 }
 
-function parseOpts(opts: any, url: string | URL) {
-  const fetchOpts = _.omit(opts, ['json', 'signal', 'basicAuth'])
-  if (opts.json) {
-    setupJsonBody(fetchOpts, opts.json)
+function parseOpts(opts: FetchRequestOptions, url: string | URL) {
+  const { json, signal, basicAuth, ...rawFetchOpts } = opts
+  const fetchOpts: ParsedFetchOptions = { ...rawFetchOpts }
+  if (json) {
+    setupJsonBody(fetchOpts, json)
   }
-  if (opts.basicAuth) {
-    setupBasicAuth(fetchOpts, opts.basicAuth)
+  if (basicAuth) {
+    setupBasicAuth(fetchOpts, basicAuth)
   }
   if (!fetchOpts.agent) {
     setupDefaultAgent(fetchOpts)
@@ -184,9 +220,9 @@ function parseOpts(opts: any, url: string | URL) {
 
   const abortController = new AbortController()
   fetchOpts.signal = abortController.signal
-  let detachSignal
-  if (opts.signal) {
-    detachSignal = abortOnSignal(abortController, opts.signal)
+  let detachSignal: () => void
+  if (signal) {
+    detachSignal = abortOnSignal(abortController, signal)
   } else {
     let overTimeoutStart: bigint | undefined
     const stack = new Error().stack
@@ -199,7 +235,7 @@ function parseOpts(opts: any, url: string | URL) {
         logger.warn(
           {
             url,
-            method: opts.method ?? 'GET',
+            method: fetchOpts.method ?? 'GET',
             overTimeoutMs:
               Number(process.hrtime.bigint() - overTimeoutStart) / 1e6,
             stack,
@@ -209,23 +245,32 @@ function parseOpts(opts: any, url: string | URL) {
       }
     }
   }
-  if (opts.body instanceof Readable) {
+  if (fetchOpts.body instanceof Readable) {
     abortOnDestroyedRequest(abortController, fetchOpts.body)
   }
   return { fetchOpts, abortController, detachSignal }
 }
 
-function setupJsonBody(fetchOpts: any, json: any) {
+function setupJsonBody(fetchOpts: ParsedFetchOptions, json: unknown) {
   fetchOpts.body = JSON.stringify(json)
-  fetchOpts.headers = fetchOpts.headers ?? {}
-  fetchOpts.headers['Content-Type'] = 'application/json'
+  if (!(fetchOpts.headers instanceof Headers)) {
+    fetchOpts.headers = new Headers(fetchOpts.headers)
+  }
+  fetchOpts.headers.set('Content-Type', 'application/json')
 }
 
-function setupBasicAuth(fetchOpts: any, basicAuth: any) {
-  fetchOpts.headers = fetchOpts.headers ?? {}
-  fetchOpts.headers.Authorization =
+function setupBasicAuth(
+  fetchOpts: ParsedFetchOptions,
+  basicAuth: BasicAuthOptions
+) {
+  if (!(fetchOpts.headers instanceof Headers)) {
+    fetchOpts.headers = new Headers(fetchOpts.headers)
+  }
+  fetchOpts.headers.set(
+    'Authorization',
     'Basic ' +
-    Buffer.from(`${basicAuth.user}:${basicAuth.password}`).toString('base64')
+      Buffer.from(`${basicAuth.user}:${basicAuth.password}`).toString('base64')
+  )
 }
 
 function abortOnSignal(abortController: AbortController, signal: AbortSignal) {
@@ -243,7 +288,7 @@ function abortOnSignal(abortController: AbortController, signal: AbortSignal) {
 
 function abortOnDestroyedRequest(
   abortController: AbortController,
-  stream: any
+  stream: Readable
 ) {
   stream.on('close', () => {
     if (!stream.readableEnded) {
@@ -265,7 +310,7 @@ function abortOnDestroyedResponse(
 
 async function performRequest(
   url: string | URL,
-  fetchOpts: any,
+  fetchOpts: ParsedFetchOptions,
   detachSignal: () => void
 ) {
   let response
@@ -283,9 +328,10 @@ async function performRequest(
   }
   response.body.on('close', detachSignal)
   if (fetchOpts.body instanceof Readable) {
+    const requestBodyStream = fetchOpts.body
     response.body.on('close', () => {
-      if (!fetchOpts.body.readableEnded) {
-        fetchOpts.body.destroy()
+      if (!requestBodyStream.readableEnded) {
+        requestBodyStream.destroy()
       }
     })
   }

libraries/fetch-utils/test/unit/FetchUtils.test.js

@@ -7,7 +7,7 @@ import fs from 'node:fs'
 import { pipeline } from 'node:stream/promises'
 import { Readable } from 'node:stream'
 import { describe, beforeAll, beforeEach, afterAll, it } from 'vitest'
-import { AbortError, FetchError } from 'node-fetch'
+import { AbortError, FetchError, Headers } from 'node-fetch'
 import selfsigned from 'selfsigned'
 import { TestServer } from './helpers/TestServer.js'
 import {
@@ -128,6 +128,15 @@ describe('fetch-utils', function () {
       expect(json).to.deep.equal({ key: 'verysecret' })
     })
 
+    it('sets an Authorization header for basic auth', async function () {
+      await fetchJson(url('/json/basic-auth'), {
+        basicAuth: { user: 'user', password: 'pass' },
+      })
+      expect(server.lastReq.headers.authorization).to.equal(
+        'Basic ' + Buffer.from('user:pass').toString('base64')
+      )
+    })
+
     it("destroys the request body if it doesn't get consumed", async function () {
       const stream = Readable.from(infiniteIterator())
       await fetchJson(url('/json/ignore-request'), {
@@ -136,6 +145,55 @@ describe('fetch-utils', function () {
       })
       expect(stream.destroyed).to.be.true
     })
+
+    describe('headers', function () {
+      it('sets an Accept header of application/json by default', async function () {
+        await fetchJson(url('/json/hello'))
+        expect(server.lastReq.headers.accept).to.equal('application/json')
+      })
+
+      it('passes provided headers', async function () {
+        await fetchJson(url('/json/hello'), {
+          headers: { 'x-some-value': 'value' },
+        })
+        expect(server.lastReq.headers['x-some-value']).to.equal('value')
+      })
+
+      it('respects an explicitly provided Accept header', async function () {
+        await fetchJson(url('/json/hello'), {
+          headers: { Accept: 'application/vnd.api+json' },
+        })
+        expect(server.lastReq.headers.accept).to.equal(
+          'application/vnd.api+json'
+        )
+      })
+
+      it('sets the default Accept header when headers are provided as a Headers instance', async function () {
+        await fetchJson(url('/json/hello'), {
+          headers: new Headers({ 'X-Foo': 'bar' }),
+        })
+        expect(server.lastReq.headers.accept).to.equal('application/json')
+        expect(server.lastReq.headers['x-foo']).to.equal('bar')
+      })
+
+      it('sets the default Accept header when headers are provided as an array of tuples', async function () {
+        await fetchJson(url('/json/hello'), {
+          headers: [['X-Foo', 'bar']],
+        })
+        expect(server.lastReq.headers.accept).to.equal('application/json')
+        expect(server.lastReq.headers['x-foo']).to.equal('bar')
+      })
+
+      it('sets a Content-Type header of application/json when sending a JSON body', async function () {
+        await fetchJson(url('/json/add'), {
+          method: 'POST',
+          json: { a: 2, b: 3 },
+        })
+        expect(server.lastReq.headers['content-type']).to.equal(
+          'application/json'
+        )
+      })
+    })
   })
 
   describe('fetchStream', function () {