lint: avoid unintented use of globals in code and tests, improve test for installing/overwriting globals (#4478)

* lint: avoid unintented use of globals  in code and tests, improve test for installing/overwriting globals

* Apply suggestions from code review

Author: Uzlopak

benchmarks/websocket/messageevent.mjs

@@ -13,6 +13,7 @@ group('MessageEvent instantiation', () => {
   })
 
   bench('global - MessageEvent init', () => {
+    // eslint-disable-next-line no-restricted-globals
     return new MessageEvent('event', { data: null, ports: [port1, port2] })
   })
 })

docs/examples/proxy/fetch.mjs

@@ -1,7 +1,7 @@
 import * as http from 'node:http'
 import { once } from 'node:events'
 import { createProxy } from 'proxy'
-import { ProxyAgent } from '../../../index.js'
+import { fetch, ProxyAgent } from '../../../'
 
 const proxyServer = createProxy(http.createServer())
 const server = http.createServer((req, res) => {

eslint.config.js

@@ -1,6 +1,7 @@
 'use strict'
 
 const neo = require('neostandard')
+const { installedExports } = require('./lib/global')
 
 module.exports = [
   ...neo({
@@ -22,7 +23,15 @@ module.exports = [
         exports: 'never',
         functions: 'never'
       }],
-      '@typescript-eslint/no-redeclare': 'off'
+      '@typescript-eslint/no-redeclare': 'off',
+      'no-restricted-globals': ['error',
+        ...installedExports.map(name => {
+          return {
+            name,
+            message: `Use undici-own ${name} instead of the global.`
+          }
+        })
+      ]
     }
   }
 ]

lib/global.js

@@ -26,7 +26,25 @@ function getGlobalDispatcher () {
   return globalThis[globalDispatcher]
 }
 
+// These are the globals that can be installed by undici.install().
+// Not exported by index.js to avoid use outside of this module.
+const installedExports = /** @type {const} */ (
+  [
+    'fetch',
+    'Headers',
+    'Response',
+    'Request',
+    'FormData',
+    'WebSocket',
+    'CloseEvent',
+    'ErrorEvent',
+    'MessageEvent',
+    'EventSource'
+  ]
+)
+
 module.exports = {
   setGlobalDispatcher,
-  getGlobalDispatcher
+  getGlobalDispatcher,
+  installedExports
 }

test/env-http-proxy-agent-nodejs-bundle.js

@@ -20,7 +20,7 @@ describe('EnvHttpProxyAgent and setGlobalDispatcher', () => {
     process.env = { ...env }
   })
 
-  test('should work together from the Node.js bundle', async (t) => {
+  test('should work with global fetch from undici bundled with Node.js', async (t) => {
     const { strictEqual } = tspl(t, { plan: 3 })
 
     // Instead of using mocks, start a real server and a minimal proxy server
@@ -69,6 +69,7 @@ describe('EnvHttpProxyAgent and setGlobalDispatcher', () => {
     process.env.http_proxy = proxyAddress
     setGlobalDispatcher(new EnvHttpProxyAgent())
 
+    // eslint-disable-next-line no-restricted-globals
     const res = await fetch(serverAddress)
     strictEqual(await res.text(), 'Hello world')
   })

test/http2.js

@@ -9,7 +9,7 @@ const { Writable, pipeline, PassThrough, Readable } = require('node:stream')
 
 const pem = require('@metcoder95/https-pem')
 
-const { Client, Agent, FormData } = require('..')
+const { Client, Agent, FormData, Response } = require('..')
 
 test('Should support H2 connection', async t => {
   const body = []

test/install.js

@@ -2,125 +2,48 @@
 
 const { test } = require('node:test')
 const assert = require('node:assert')
-const { install } = require('../index')
+const undici = require('../index')
+const { installedExports } = require('../lib/global')
 
-test('install() should add WHATWG fetch classes to globalThis', () => {
-  // Save original globals to restore later
-  const originalFetch = globalThis.fetch
-  const originalHeaders = globalThis.Headers
-  const originalResponse = globalThis.Response
-  const originalRequest = globalThis.Request
-  const originalFormData = globalThis.FormData
-  const originalWebSocket = globalThis.WebSocket
-  const originalCloseEvent = globalThis.CloseEvent
-  const originalErrorEvent = globalThis.ErrorEvent
-  const originalMessageEvent = globalThis.MessageEvent
-  const originalEventSource = globalThis.EventSource
+test('install() should overwrite only specified elements in globalThis', () => {
+  const exportsToCheck = new Set(installedExports)
 
-  try {
-    // Remove any existing globals
-    delete globalThis.fetch
-    delete globalThis.Headers
-    delete globalThis.Response
-    delete globalThis.Request
-    delete globalThis.FormData
-    delete globalThis.WebSocket
-    delete globalThis.CloseEvent
-    delete globalThis.ErrorEvent
-    delete globalThis.MessageEvent
-    delete globalThis.EventSource
+  // Use a Proxy to verify that only the expected globals are set
+  // and no other properties are added to globalThis by undici.install().
+  const proxyGlobalThis = new Proxy(globalThis, {
+    set (target, prop, value) {
+      if (exportsToCheck.has(prop)) {
+        target[prop] = value
+        exportsToCheck.delete(prop)
+        return true
+      }
+      throw new Error(`Unexpected global set: ${String(prop)}`)
+    }
+  })
 
-    // Verify they're not defined
-    assert.strictEqual(globalThis.fetch, undefined)
-    assert.strictEqual(globalThis.Headers, undefined)
-    assert.strictEqual(globalThis.Response, undefined)
-    assert.strictEqual(globalThis.Request, undefined)
-    assert.strictEqual(globalThis.FormData, undefined)
-    assert.strictEqual(globalThis.WebSocket, undefined)
-    assert.strictEqual(globalThis.CloseEvent, undefined)
-    assert.strictEqual(globalThis.ErrorEvent, undefined)
-    assert.strictEqual(globalThis.MessageEvent, undefined)
-    assert.strictEqual(globalThis.EventSource, undefined)
+  // eslint-disable-next-line no-global-assign
+  globalThis = proxyGlobalThis
 
-    // Call install()
-    install()
+  undici.install()
 
-    // Verify all classes are now installed
-    assert.strictEqual(typeof globalThis.fetch, 'function')
-    assert.strictEqual(typeof globalThis.Headers, 'function')
-    assert.strictEqual(typeof globalThis.Response, 'function')
-    assert.strictEqual(typeof globalThis.Request, 'function')
-    assert.strictEqual(typeof globalThis.FormData, 'function')
-    assert.strictEqual(typeof globalThis.WebSocket, 'function')
-    assert.strictEqual(typeof globalThis.CloseEvent, 'function')
-    assert.strictEqual(typeof globalThis.ErrorEvent, 'function')
-    assert.strictEqual(typeof globalThis.MessageEvent, 'function')
-    assert.strictEqual(typeof globalThis.EventSource, 'function')
+  assert.strictEqual(exportsToCheck.size, 0, `Some expected globals were not set: ${[...exportsToCheck].join(', ')}`)
 
-    // Test that the installed classes are functional
-    const headers = new globalThis.Headers([['content-type', 'application/json']])
-    assert.strictEqual(headers.get('content-type'), 'application/json')
+  // Verify that the installed globals match the exports from undici
+  for (const name of installedExports) {
+    assert.strictEqual(globalThis[name], undici[name])
+  }
 
-    const request = new globalThis.Request('https://example.com')
-    assert.strictEqual(request.url, 'https://example.com/')
+  // Test that the installed classes are functional
+  const headers = new globalThis.Headers([['content-type', 'application/json']])
+  assert.strictEqual(headers.get('content-type'), 'application/json')
 
-    const response = new globalThis.Response('test body')
-    assert.strictEqual(response.status, 200)
+  const request = new globalThis.Request('https://example.com')
+  assert.strictEqual(request.url, 'https://example.com/')
 
-    const formData = new globalThis.FormData()
-    formData.append('key', 'value')
-    assert.strictEqual(formData.get('key'), 'value')
-  } finally {
-    // Restore original globals
-    if (originalFetch !== undefined) {
-      globalThis.fetch = originalFetch
-    } else {
-      delete globalThis.fetch
-    }
-    if (originalHeaders !== undefined) {
-      globalThis.Headers = originalHeaders
-    } else {
-      delete globalThis.Headers
-    }
-    if (originalResponse !== undefined) {
-      globalThis.Response = originalResponse
-    } else {
-      delete globalThis.Response
-    }
-    if (originalRequest !== undefined) {
-      globalThis.Request = originalRequest
-    } else {
-      delete globalThis.Request
-    }
-    if (originalFormData !== undefined) {
-      globalThis.FormData = originalFormData
-    } else {
-      delete globalThis.FormData
-    }
-    if (originalWebSocket !== undefined) {
-      globalThis.WebSocket = originalWebSocket
-    } else {
-      delete globalThis.WebSocket
-    }
-    if (originalCloseEvent !== undefined) {
-      globalThis.CloseEvent = originalCloseEvent
-    } else {
-      delete globalThis.CloseEvent
-    }
-    if (originalErrorEvent !== undefined) {
-      globalThis.ErrorEvent = originalErrorEvent
-    } else {
-      delete globalThis.ErrorEvent
-    }
-    if (originalMessageEvent !== undefined) {
-      globalThis.MessageEvent = originalMessageEvent
-    } else {
-      delete globalThis.MessageEvent
-    }
-    if (originalEventSource !== undefined) {
-      globalThis.EventSource = originalEventSource
-    } else {
-      delete globalThis.EventSource
-    }
-  }
+  const response = new globalThis.Response('test body')
+  assert.strictEqual(response.status, 200)
+
+  const formData = new globalThis.FormData()
+  formData.append('key', 'value')
+  assert.strictEqual(formData.get('key'), 'value')
 })

test/node-fetch/request.js

@@ -5,7 +5,7 @@ const { describe, it, before, after } = require('node:test')
 const stream = require('node:stream')
 const http = require('node:http')
 
-const { Request } = require('../../index.js')
+const { Request, FormData } = require('../../index.js')
 const TestServer = require('./utils/server.js')
 
 describe('Request', () => {

test/redirect-request.js

@@ -235,6 +235,7 @@ for (const factory of [
 
     const { statusCode, headers, body: bodyStream } = await request(t, server, undefined, `http://${server}/303`, {
       method: 'PATCH',
+      // eslint-disable-next-line no-restricted-globals
       headers: new Headers({
         'Content-Encoding': 'gzip',
         'X-Foo1': '1',

test/wpt/start-websockets.mjs

@@ -7,6 +7,7 @@ import { on } from 'events'
 const { WPT_REPORT } = process.env
 
 function isGlobalAvailable () {
+  // eslint-disable-next-line no-restricted-globals
   if (typeof WebSocket !== 'undefined') {
     return true
   }