Use an isomorphic fetching library

[ztcollazo]

I've recently been trying to use an edge environment to deploy a project of mine, and the main error that I get is: Cannot bundle Node.js built-in "stream" imported from "node_modules/@octokit/request/node_modules/node-fetch/lib/index.mjs". However, edge environments do not support Node.js, even though I believed this was an isomorphic library. Apparently, node-fetch is not isomorphic. I don't know how hard this would be or how much time it would take, but I could also open a PR if necessary.

[safinsingh]

A library like cross-fetch would be suitable for this (I believe that isomorphic-fetch is no longer maintained). One issue I ran into while trying to replace the library 1-for-1 is that cross-fetch does not have the HeadersInit type exported. This could be fixed by simply including a definition for it within this library. Let me know if I should make a PR for this.

[ztcollazo]

It looks to me like cross-fetch uses node-fetch in the background for Node.js. I'm not sure if tree-shaking will filter this out or not, because this repo is supposed to be isomorphic also. However, it may be better because it's exporting different values based on the environment, whereas request.js is always using node-fetch unless a different fetch is passed (or so it looks to me).

[wolfy1339]

You can specify your own fetch method by setting the options.request.fetch to the fetch function you want to use.

That can alleviate some of your concerns and allow you to use the proper fetch

[baoshan]

From the error message, it seems the bundler is complaining about importing node-fetch.

I guess you’re using Netlify edge functions, which support importing absolute path. esm.sh replaces node-fetch with node-fetch-native, you can verify it here: https://esm.sh/v86/@octokit/request@6.0.2/es2021/request.js

[gr2m]

we don't want to add an isomorphic fetching library as dependency because it would massively increase the bundle size. We want to stay close the web platform, as that's where all the JS runtime environments gravitate towards. fetch is available in most environments, including the Node 18+. We might move away from even including node-fetch in the future and suggesting to use whatever polyfil works for your environment, or ask you to pass your own fetch polyfill as request.fetch option as suggested by @wolfy1339 above

[ztcollazo]

That probably is the best way to go. The only reason that I mention this issue is that I was having trouble with node-fetch in an edge environment, so until that is somehow changed, I will have to use a Node environment. I did check out node-fetch-native that @baoshan mentioned, and it seemed pretty nice. I may be able to try using NPM overrides to force this project to use it until it no longer includes node-fetch. From my understanding, it has the same API, so it shouldn't break anything.

[gr2m]

an alternative, if you only use @octokit/request, would be to use @octokit/endpoint instead. It's entirely unopinionated on how you send the request, the endpoint function just gives you generic request options that you can pass to your request method of choice. There is no node-fetch in the dependency tree for @octokit/endpoint

[ztcollazo]

I'm using @octokit/graphql, but the stack trace showed me that it was @octokit/request erroring. I may be able to completely rewrite the graphql query interface, but it would seem that @octokit/endpoint wouldn't be helpful in that regard, as it is only for generating the request options for the REST endpoints. I'll keep that in mind, though.

[styfle]

fetch is available in most environments, including the Node 18+. We might move away from even including node-fetch in the future and suggesting to use whatever polyfil works for your environment

@gr2m That sounds like a great idea!

That would fix a ton of issues and make oktokit compatible with many more runtimes!

When do you want to ship this change? Do you want me to create a PR?

[wolfy1339]

It would be a breaking change, so best to coordinate with other breaking changes as needed.

I would say to wait for the ESM rewrite, however that seems to be on standby currently.

Feel free to send a PR and we can go from there

[styfle]

@wolfy1339 I created PR #580

[tom-sherman]

The dependency on node-fetch also makes this package difficult to include in Next.js projects deployed to Vercel. Here's the error I get:

./node_modules/.pnpm/node-fetch@2.6.11/node_modules/node-fetch/lib/index.js
Module not found: Can't resolve 'encoding' in '/vercel/path0/node_modules/.pnpm/node-fetch@2.6.11/node_modules/node-fetch/lib'
Import trace for requested module:
./node_modules/.pnpm/node-fetch@2.6.11/node_modules/node-fetch/lib/index.js
./node_modules/.pnpm/@octokit+request@6.2.3/node_modules/@octokit/request/dist-node/index.js
./src/app/api/github-webhook/route.ts

Removing the dependency on node-fetch should fix it.

Edit: Can confirm when I patched out the import to node-fetch then the build succeeded.

[github-actions]

🎉 This issue has been resolved in version 7.0.0-beta.2 🎉

The release is available on:

• GitHub release
• npm package (@beta dist-tag)

Your semantic-release bot 📦🚀

[styfle]

This can be closed now that #580 landed

[wolfy1339]

Let's keep it open until it reaches the final stable release

[gr2m]

it will be closed once beta is merged into main

[github-actions]

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀