Skip to content
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.

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

Replace node-fetch with Node.js' native fetch #777

Conversation

Kurre
Copy link
Contributor

@Kurre Kurre commented Jun 13, 2024

  • 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

Copy link

netlify bot commented Jun 13, 2024

Deploy Preview for testcontainers-node ready!

Name Link
🔨 Latest commit 0a8e473
🔍 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.

@Kurre Kurre changed the title Replace node-fetch in favor of Node.js' native fetch Replace node-fetch in with Node.js' native fetch Jun 13, 2024
@Kurre Kurre changed the title Replace node-fetch in with Node.js' native fetch Replace node-fetch with Node.js' native fetch Jun 13, 2024
@cristianrgreco cristianrgreco added maintenance Improvements that do not change functionality patch Backward compatible bug fix labels Jun 14, 2024
@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Jun 15, 2024

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?

@Kurre
Copy link
Contributor Author

Kurre commented Jun 17, 2024

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 Kurre force-pushed the feat/replace-node-fetch-with-native-fetch branch from b8a76f9 to c289d84 Compare June 24, 2024 08:23
@Kurre
Copy link
Contributor Author

Kurre commented Jun 24, 2024

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

@Kurre
Copy link
Contributor Author

Kurre commented Jun 25, 2024

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.

@cristianrgreco cristianrgreco added minor Backward compatible functionality and removed patch Backward compatible bug fix labels Jun 30, 2024
@Kurre Kurre force-pushed the feat/replace-node-fetch-with-native-fetch branch 2 times, most recently from 128e713 to 8ac3eac Compare July 3, 2024 10:53
@Kurre
Copy link
Contributor Author

Kurre commented Jul 3, 2024

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

@cristianrgreco
Copy link
Collaborator

cristianrgreco commented Jul 12, 2024

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?

Kurre added 5 commits July 14, 2024 11:42
- 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`
@Kurre
Copy link
Contributor Author

Kurre commented Jul 14, 2024

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 Kurre force-pushed the feat/replace-node-fetch-with-native-fetch branch from 92ff8c8 to 5058c5a Compare July 14, 2024 12:26
@cristianrgreco cristianrgreco merged commit fe7b00a into testcontainers:main Jul 27, 2024
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Improvements that do not change functionality minor Backward compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node22 DeprecationWarning due node-fetch 2.x
2 participants