testcontainers-node icon indicating copy to clipboard operation
testcontainers-node copied to clipboard

Replace `node-fetch` with Node.js' native `fetch`

Open Kurre opened this issue 1 year ago • 6 comments

  • Remove node-fetch and @types/node-fetch packages
  • Replace all fetch() calls to use native fetch
  • Replace node-fetch's timeout with AbortSignal
  • Replace https.Agent with Agent from undici
  • Add ES2022 as lib to tsconfig.base.json to have correct types for fetch

closes #765

Kurre avatar Jun 13 '24 05:06 Kurre

Deploy Preview for testcontainers-node ready!

Name Link
Latest commit 0a8e47382a347859c4c60267c0076476b7d361e9
Latest deploy log https://app.netlify.com/sites/testcontainers-node/deploys/66a3e3a82d9cc900085227ca
Deploy Preview https://deploy-preview-777--testcontainers-node.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jun 13 '24 05:06 netlify[bot]

Hi @Kurre, thanks for raising. I see that the Agent is being imported from undici but that dependency is not installed. Is there a reason you're using that module instead of the https builtin? This is why the build is failing. There are also some lint issues. Could you please have a look?

cristianrgreco avatar Jun 15 '24 07:06 cristianrgreco

Hi @Kurre, thanks for raising. I see that the Agent is being imported from undici but that dependency is not installed. Is there a reason you're using that module instead of the https builtin? This is why the build is failing. There are also some lint issues. Could you please have a look?

Yes, the reason is that the Agent from http/https module isn't API compatible with fetch. Totally forgot to check if the undici's Agent is exposed in Node, since undici is an internal dep.

AFAIK there isn't other ways to properly handle this situation with fetch 🤔 So probably options are either to include the undici package as dependency or find some other solution 🤔

Kurre avatar Jun 17 '24 13:06 Kurre

fixed lint issues and added the undici as dependency, since there seems to be no proper way to use agent/dispatcher without it 😕

Kurre avatar Jun 24 '24 09:06 Kurre

fixed lint issues and added the undici as dependency, since there seems to be no proper way to use agent/dispatcher without it 😕

I used the undici@^5.28.4 which works with Node.js 18, 20 and 22, even though v20 and v22 use undici 6.x as an internal dependency.

afaik dropping the Node.js v16 support was the only breaking change of v6.x, https://github.com/nodejs/undici/releases/tag/v6.0.0 and it seems that v6.x has extended some interfaces (like Dispatcher), but otherwise fully compatible.

We could also dynamically import the right undici version (5.x to v18 and 6.x to v20/v22) which would probably also work just fine, but would probably require some tweaking with the typings 🤔

Any thoughts on this @cristianrgreco ? Personally i'm open for all possible routes forward, since replacing the v2.x node-fetch is probably inevitable.

Kurre avatar Jun 25 '24 09:06 Kurre

fixed lint issues and added the undici as dependency, since there seems to be no proper way to use agent/dispatcher without it 😕

I used the undici@^5.28.4 which works with Node.js 18, 20 and 22, even though v20 and v22 use undici 6.x as an internal dependency.

afaik dropping the Node.js v16 support was the only breaking change of v6.x, https://github.com/nodejs/undici/releases/tag/v6.0.0 and it seems that v6.x has extended some interfaces (like Dispatcher), but otherwise fully compatible.

We could also dynamically import the right undici version (5.x to v18 and 6.x to v20/v22) which would probably also work just fine, but would probably require some tweaking with the typings 🤔

Any thoughts on this @cristianrgreco ? Personally i'm open for all possible routes forward, since replacing the v2.x node-fetch is probably inevitable.

@types/node seems to be using v5.x types for undici, https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/package.json#L12 & https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/globals.d.ts#L6-L13

Kurre avatar Jul 03 '24 11:07 Kurre

Thanks @Kurre. I'm not thrilled with the addition of undici as a dep, is there a way to do without? E.g:

import https from 'https';

const httpsAgent = new https.Agent({
  rejectUnauthorized: false,
});

const response = await fetch(url, {
  ...
  agent: httpsAgent,
});

Also was there a reason to add ES2022 to the libs?

cristianrgreco avatar Jul 12 '24 16:07 cristianrgreco

Thanks @Kurre. I'm not thrilled with the addition of undici as a dep, is there a way to do without? E.g:

import https from 'https';

const httpsAgent = new https.Agent({
  rejectUnauthorized: false,
});

const response = await fetch(url, {
  ...
  agent: httpsAgent,
});

Also was there a reason to add ES2022 to the libs?

I'd like to avoid new deps if possible, but so far I haven't found a proper solution for it when using fetch. https.Agent isn't compatible with fetch, I created a simple test script for verify this:

const undici = require("undici");
const https = require("node:https");

const url = "https://self-signed.badssl.com";
const httpsAgent = new https.Agent({ rejectUnauthorized: false });
const undiciAgent = new undici.Agent({ connect: { rejectUnauthorized: false } });

fetch(url)
  .then((res) => console.log("\n\nres", res))
  .catch((err) => console.error("\n\nerror", err));

fetch(url, { agent: httpsAgent })
  .then((res) => console.log("\n\nres with httpsAgent", res))
  .catch((err) => console.error("\n\nerror with fetchWithAgent", err));

fetch(url, { dispatcher: undiciAgent })
  .then((res) => console.log("\n\nres with dispatcher", res))
  .catch((err) => console.error("\n\nerror with dispatcher", err));

and output is as follows:

❯ node node-fetch-test.js


error TypeError: fetch failed
    at node:internal/deps/undici/undici:12502:13
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  [cause]: Error: self-signed certificate
      at TLSSocket.onConnectSecure (node:_tls_wrap:1674:34)
      at TLSSocket.emit (node:events:519:28)
      at TLSSocket._finishInit (node:_tls_wrap:1085:8)
      at ssl.onhandshakedone (node:_tls_wrap:871:12) {
    code: 'DEPTH_ZERO_SELF_SIGNED_CERT'
  }
}


error with fetchWithAgent TypeError: fetch failed
    at node:internal/deps/undici/undici:12502:13
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  [cause]: Error: self-signed certificate
      at TLSSocket.onConnectSecure (node:_tls_wrap:1674:34)
      at TLSSocket.emit (node:events:519:28)
      at TLSSocket._finishInit (node:_tls_wrap:1085:8)
      at ssl.onhandshakedone (node:_tls_wrap:871:12) {
    code: 'DEPTH_ZERO_SELF_SIGNED_CERT'
  }
}


res with dispatcher Response {
  status: 200,
  statusText: 'OK',
  headers: Headers {
    server: 'nginx/1.10.3 (Ubuntu)',
    date: 'Sun, 14 Jul 2024 09:08:13 GMT',
    'content-type': 'text/html',
    'last-modified': 'Fri, 17 May 2024 17:59:55 GMT',
    'transfer-encoding': 'chunked',
    connection: 'keep-alive',
    etag: 'W/"66479b1b-1f6"',
    'cache-control': 'no-store',
    'content-encoding': 'gzip'
  },
  body: ReadableStream { locked: false, state: 'readable', supportsBYOB: true },
  bodyUsed: false,
  ok: true,
  redirected: false,
  type: 'basic',
  url: 'https://self-signed.badssl.com/'
}

and the ES2022 on libs is needed to have correct types for fetch, without it it'd use types of DOM's fetch. Maybe lower spec would also work, i'll need to test that one.

edit: As the target in set to es2022 TS should update the lib, https://www.typescriptlang.org/tsconfig/#target. But at least VSCode doesn't handle types correctly without lib being set.

Kurre avatar Jul 14 '24 09:07 Kurre