BREAKING CHANGE: remove node-fetch in favor of global

[styfle]

• Resolves Use an isomorphic fetching library #456
• Resolves fix(deps): update dependency node-fetch to v3 #422
• Resolves pin version of node-fetch and add end-to-end test to catch breaking changes #432

---

Behavior

Before the change?

Previously you could run this on Node.js 14 and 16 without manually polyfilling fetch.

After the change?

With this PR it will work in Node.js 18, browsers, and any JS runtime that provides fetch. If you want to run in Node.js 14 or 16, you can polyfill globalThis.fetch or pass in the request options.

Other information

This improves compatibility with many runtimes because node-fetch is not the same as the standard fetch global.

---

Additional info

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes (Please add the Type: Breaking change label)

If Yes, what's the impact:

If you want to run in Node.js 14 or 16, you must polyfill globalThis.fetch or pass in the request options.

Pull request type

Please add the corresponding label for change this PR introduces:

• Feature/model/API additions: Type: Feature

---

[kroese]

You can also add this to package.json instead:

"overrides": {
    "node-fetch": "npm:node-fetch-native@latest"
  },

That way it doesnt require any changes to the existing code, and it will not be a breaking change anymore.

[wolfy1339]

Can you fix the tests for Node 14 and 16?

[wolfy1339]

I think adding a simple patch like so will work:

(async function () {
  if (parseInt(process.versions.node.split(".")[0]) <= 18) {
    // @ts-expect-error
    globalThis.fetch = await import("node-fetch");
  }
})();

[kroese]

@wolfy1339 I think that way bundlers will still include node-fetch, because they cannot evaluate the if-expression during bundling.

With the solution I suggested I reduced the size of my app that requires octokit significantly.

[wolfy1339]

The patch shouldn't go in any production code, just for the test files.

[kroese]

The patch shouldn't go in any production code, just for the test files.

Okay, sorry I misunderstood. But I still think nodefetch-native would be an easier solution. It uses the native fetch when available (node 18+) and uses nodefetch 3.0 as a fallback for lower versions. This would allow request.js to transition to native fetch without breaking anything for older versions.

I am using request.js with it without any problems.

[styfle]

@wolfy1339 I used a similar solution but with require() so its synchronous a little less code.

I confirmed the tests are now passing on node 16 and 18.

[wolfy1339]

Even with node-fetch-native it wouldn't be a single change.

As node-fetch v3 drops the timeout option since it isn't in the spec, the types and code need updating.

[kroese]

Even with node-fetch-native it wouldn't be a single change.

As node-fetch v3 drops the timeout option since it isn't in the spec, the types and code need updating.

I didn't notice anything about timeouts, all I can say is that I didn't notice any change in behaviour when using node-fetch-native but maybe there are some certain edge-cases where it doesn't behave the same.

[wolfy1339]

We are using node-fetch v2 at the moment which had the timeout option.
In v3 it was removed.

https://github.com/node-fetch/node-fetch/blob/main/docs/v3-UPGRADE-GUIDE.md#the-timeout-option-was-removed

[gr2m]

I would remove the dependency on node-fetch altogether. That's what we already did in octokit-next:
https://github.com/octokit/octokit-next.js/pull/62/files

A custom fetch function can be configured already. For users of older Node versions, we can document how to use node-fetch instead.

Node 16 is the last version that doesn't have it built in, and it will be out of maintenance mode in September. After that all current JavaScript runtime environments have the global fetch method. It might make sense to remove support for both Node 14 & Node 16 at this point, and while doing that also remove the dependency on node-fetch

[wolfy1339]

We'll need to remove the timeout option as well, since it doesn't exist on the fetch API or newer versions of node-fetch.

[styfle]

There is a similar solution for timeout using

AbortSignal.timeout(opts.timeout)

Although if the consumer provides their own fetch then they can provider their own options too, so we can remove the options now that I think about it.

Let me know how to proceed.

[wolfy1339]

Since this is going to be a breaking change, let's just remove that option from our code altogether

[gr2m]

note that this will also be a breaking change for all the packages that depend on @octokit/request. If possible, I'd try to coordinate with other breaking changes in the dependents.

[wolfy1339]

Remove timeout parameter tests

[styfle]

Removed it from the tests in ea7c2ab

[wolfy1339]

These tests don't really seem useful now. Could you remove them

[styfle]

Sounds good! Removed in 3cfc7c8

[wolfy1339]

Will this resolve #432 @gr2m as node-fetch won't be included anymore

[gr2m]

Will this resolve #432 @gr2m as node-fetch won't be included anymore

yes 🚀

[gr2m]

I would recommend we also remove the request.timeout option while at it. We already recommend options.request.signal today

[styfle]

Looks like I have some merge conflicts now. Do you prefer git rebase or git merge to resolve?

[wolfy1339]

Either way is fine. We squash and merge anyways in the end.

[gr2m]

Merge is easier to keep track of new changes for reviewers.

[styfle]

@wolfy1339 @gr2m I fixed the merge conflicts and addressed the remaining comments, thanks!

[wolfy1339]

There are some linting issues left to fix.

[gr2m]

Great work, just some lint errors left:
https://github.com/octokit/request.js/actions/runs/5039405053/jobs/9038110405?pr=580#step:5:7

running npm run lint:fix should do the trick

[gr2m]

Wow this is a big day for Octokit's cross plattform support. Thank you all for making this a reality ❤️

[github-actions]

🎉 This PR is included in version 7.0.0-beta.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀