feat(fetch): use undici + ProxyAgent (#1000)

| 🚥 Resolves RM-8008, RM-9687 | | :------------------- | ## 🧰 Changes - [x] Removes `node-fetch` in favor of its native Node.js counterpart - [x] Removes `formdata-node` in favor of its native Node.js counterpart - [x] Uses undici's [`ProxyAgent`](https://undici.nodejs.org/#/docs/api/ProxyAgent) for proxying requests[^1] - [x] Now that we installed the latest `nock` beta in #999, `msw` is no longer required and has been removed. - [x] Removed a few useless `delayConnection` statements so now tests are like 2-3x faster ## 🧬 QA & Testing Do tests still pass? [^1]: We very well might have been doing proxying incorrectly this entire time? Ideally we should use the new [`EnvHttpProxyAgent`](https://github.com/nodejs/undici/blob/main/docs/docs/api/EnvHttpProxyAgent.md) API instead but that's only supported in `undici@6` 😔
readmeio · May 7, 2024 · 9da7132 · 9da7132
1 parent 35bf8e9
commit 9da7132
Show file tree

Hide file tree

Showing 22 changed files with 170 additions and 809 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -55,11 +55,6 @@
       "error",
       {
         "paths": [
-          {
-            "name": "node-fetch",
-            "importNames": ["default"],
-            "message": "Avoid using `node-fetch` directly and instead use the fetch wrapper located in `lib/readmeAPIFetch.ts`. See CONTRIBUTING.md for more information."
-          },
           {
             "name": "ci-info",
             "message": "The `ci-info` package is difficult to test because misleading results will appear when running tests in the GitHub Actions runner. Instead of importing this package directly, create a wrapper function in `lib/isCI.ts` and import that instead."

diff --git a/.github/dependabot.yml b/.github/dependabot.yml
@@ -37,3 +37,10 @@ updates:
       - dependency-name: ora
         versions:
           - '>= 7'
+      # There are ProxyAgent discrepancies between undici@6 and
+      # the Node.js fetch implementation (which uses undici@5).
+      # Until we use undici itself for fetch calls, we should
+      # pin undici to the version used in Node.js core.
+      - dependency-name: undici
+        versions:
+          - '>= 6'
diff --git a/__tests__/cmds/openapi/index.test.ts b/__tests__/cmds/openapi/index.test.ts
@@ -2,8 +2,6 @@
 import fs from 'node:fs';
 
 import chalk from 'chalk';
-import { http } from 'msw';
-import { setupServer } from 'msw/node';
 import nock from 'nock';
 import prompts from 'prompts';
 import { describe, beforeAll, beforeEach, afterEach, it, expect, vi } from 'vitest';
@@ -135,7 +133,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -165,7 +162,6 @@ describe('rdme openapi', () => {
 
       const postMock = getAPIMockWithVersionHeader(version)
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -196,7 +192,6 @@ describe('rdme openapi', () => {
 
       const postMock = getAPIMockWithVersionHeader(version)
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -416,7 +411,6 @@ describe('rdme openapi', () => {
           { _id: 'spec2', title: 'spec2_title' },
         ])
         .put('/api/v1/api-specification/spec2', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -484,7 +478,6 @@ describe('rdme openapi', () => {
           .basicAuth({ user: key })
           .reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
           .put('/api/v1/api-specification/spec1', { registryUUID })
-          .delayConnection(1000)
           .basicAuth({ user: key })
           .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -544,7 +537,6 @@ describe('rdme openapi', () => {
           .post('/api/v1/api-registry', body => body.match('form-data; name="spec"'))
           .reply(201, { registryUUID, spec: { openapi: '3.0.0' } })
           .put('/api/v1/api-specification/spec1', { registryUUID })
-          .delayConnection(1000)
           .basicAuth({ user: key })
           .reply(function (uri, rBody, cb) {
             expect(this.req.headers['x-readme-version']).toBeUndefined();
@@ -824,7 +816,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(400, errorObject);
 
@@ -856,7 +847,6 @@ describe('rdme openapi', () => {
 
       const putMock = getAPIMockWithVersionHeader(version)
         .put(`/api/v1/api-specification/${id}`, { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(400, errorObject);
 
@@ -922,7 +912,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(400, errorObject);
 
@@ -949,7 +938,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(400, 'some non-JSON upload error');
 
@@ -980,7 +968,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(500, '<title>Application Error</title>');
 
@@ -1028,7 +1015,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -1071,7 +1057,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -1118,7 +1103,6 @@ describe('rdme openapi', () => {
           { _id: 'spec2', title: 'spec2_title' },
         ])
         .put('/api/v1/api-specification/spec2', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 'spec2' }, { location: exampleRefLocation });
 
@@ -1155,7 +1139,6 @@ describe('rdme openapi', () => {
 
       const mockWithHeader = getAPIMockWithVersionHeader(altVersion)
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -1192,7 +1175,6 @@ describe('rdme openapi', () => {
 
       const postMock = getAPIMockWithVersionHeader(version)
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -1232,7 +1214,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
         .put('/api/v1/api-specification/spec1', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -1270,7 +1251,6 @@ describe('rdme openapi', () => {
         .basicAuth({ user: key })
         .reply(200, [{ _id: 'spec1', title: 'spec1_title' }])
         .post('/api/v1/api-specification', { registryUUID })
-        .delayConnection(1000)
         .basicAuth({ user: key })
         .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
@@ -1350,37 +1330,24 @@ describe('rdme openapi', () => {
     });
 
     it('should send proper headers in GitHub Actions CI for spec hosted at URL', async () => {
-      expect.assertions(8);
       const registryUUID = getRandomRegistryId();
       const spec = 'https://example.com/openapi.json';
 
-      // TODO: move all of this boilerplate to the top-level once we migrate everything over to MSW
-      const server = setupServer(...[]);
+      const mock = getAPIMock()
+        .post('/api/v1/api-registry', body => body.match('form-data; name="spec"'))
+        .reply(201, { registryUUID });
 
-      server.listen({ onUnhandledRequest: 'error' });
+      const exampleMock = nock('https://example.com').get('/openapi.json').reply(200, petstoreWeird);
 
-      server.use(
-        ...[
-          http.post(`${config.host}/api/v1/api-registry`, async ({ request }) => {
-            const body = await request.text();
-            expect(body).toMatch('form-data; name="spec"');
-            return Response.json({ registryUUID }, { status: 201 });
-          }),
-          http.get(spec, () => {
-            return Response.json(petstoreWeird, { status: 200 });
-          }),
-          http.put(`${config.host}/api/v1/api-specification/${id}`, async ({ request }) => {
-            expect(request.headers.get('authorization')).toBeBasicAuthApiKey(key);
-            expect(request.headers.get('x-rdme-ci')).toBe('GitHub Actions (test)');
-            expect(request.headers.get('x-readme-source')).toBe('cli-gh');
-            expect(request.headers.get('x-readme-source-url')).toBe(spec);
-            expect(request.headers.get('x-readme-version')).toBe(version);
-            const body = await request.json();
-            expect(body).toStrictEqual({ registryUUID });
-            return Response.json({ _id: 1 }, { status: 201, headers: { location: exampleRefLocation } });
-          }),
-        ],
-      );
+      const putMock = getAPIMock({
+        'x-rdme-ci': 'GitHub Actions (test)',
+        'x-readme-source': 'cli-gh',
+        'x-readme-source-url': spec,
+        'x-readme-version': version,
+      })
+        .put(`/api/v1/api-specification/${id}`, { registryUUID })
+        .basicAuth({ user: key })
+        .reply(201, { _id: 1 }, { location: exampleRefLocation });
 
       await expect(
         openapi.run({
@@ -1391,7 +1358,9 @@ describe('rdme openapi', () => {
         }),
       ).resolves.toBe(successfulUpdate(spec));
 
-      return server.resetHandlers();
+      putMock.done();
+      exampleMock.done();
+      return mock.done();
     });
   });
 });
diff --git a/__tests__/helpers/get-api-mock.ts b/__tests__/helpers/get-api-mock.ts
@@ -1,25 +1,15 @@
-import type { Headers } from 'node-fetch';
-
-import { http } from 'msw';
 import nock from 'nock';
 
 import config from '../../src/lib/config.js';
 import { getUserAgent } from '../../src/lib/readmeAPIFetch.js';
 
-/**
- * A type describing a raw object of request headers.
- * We use this in our API request mocking to validate that the request
- * contains all the expected headers.
- */
-type ReqHeaders = Record<string, unknown>;
-
 /**
  * Nock wrapper that adds required `user-agent` request header
  * so it gets properly picked up by nock.
  * @param proxy Optional proxy URL. Must contain trailing slash.
  */
-export default function getAPIMock(reqHeaders = {}, proxy = '') {
-  return nock(`${proxy}${config.host}`, {
+export default function getAPIMock(reqHeaders = {}) {
+  return nock(config.host, {
     reqheaders: {
       'User-Agent': getUserAgent(),
       ...reqHeaders,
@@ -32,62 +22,3 @@ export function getAPIMockWithVersionHeader(v: string) {
     'x-readme-version': v,
   });
 }
-
-function validateHeaders(headers: Headers, basicAuthUser: string, expectedReqHeaders: ReqHeaders) {
-  // validate all headers in expectedReqHeaders
-  Object.keys(expectedReqHeaders).forEach(reqHeaderKey => {
-    if (headers.get(reqHeaderKey) !== expectedReqHeaders[reqHeaderKey]) {
-      throw new Error(
-        `Expected the request header '${expectedReqHeaders[reqHeaderKey]}', received '${headers.get(reqHeaderKey)}'`,
-      );
-    }
-  });
-
-  // validate basic auth header
-  if (basicAuthUser) {
-    const encodedApiKey = headers.get('Authorization').split(' ')[1];
-    const decodedApiKey = Buffer.from(encodedApiKey, 'base64').toString();
-    if (decodedApiKey !== `${basicAuthUser}:`) {
-      throw new Error(`Expected API key '${basicAuthUser}', received '${decodedApiKey}'`);
-    }
-  }
-
-  const userAgent = headers.get('user-agent');
-  if (userAgent !== getUserAgent()) {
-    throw new Error(`Expected user agent '${getUserAgent()}', received '${userAgent}'`);
-  }
-}
-
-export function getAPIMockMSW(
-  /**
-   * API route to mock against, must start with slash
-   * @example /api/v1
-   */
-  path: string = '',
-  status = 200,
-  response?: { json?: unknown; text?: string },
-  /**
-   * A string which represents the user that's passed via basic authentication.
-   * In our case, this will almost always be the user's ReadMe API key.
-   */
-  basicAuthUser = '',
-  /** Any request headers that should be matched. */
-  expectedReqHeaders: ReqHeaders = {},
-  proxy = '',
-) {
-  return http.get(`${proxy}${config.host}${path}`, ({ request }) => {
-    try {
-      // @ts-expect-error once we move off node-fetch, we can make these types consistent
-      validateHeaders(request.headers, basicAuthUser, expectedReqHeaders);
-      let httpResponse = new Response(null, { status });
-      if (response?.json) {
-        httpResponse = Response.json(response.json, { status });
-      } else if (response?.text) {
-        httpResponse = new Response(response.text, { status });
-      }
-      return httpResponse;
-    } catch (e) {
-      throw new Error(`Error mocking GET request to https://dash.readme.com${path}: ${e.message}`);
-    }
-  });
-}