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

BREAKING CHANGE: remove node-fetch in favor of global #580

Merged
merged 11 commits into from
May 21, 2023
5 changes: 2 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ to interact with [GitHub’s REST API](https://developer.github.com/v3/) and
[GitHub’s GraphQL API](https://developer.github.com/v4/guides/forming-calls/#the-graphql-endpoint).

It uses [`@octokit/endpoint`](https://github.com/octokit/endpoint.js) to parse
the passed options and sends the request using [fetch](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API)
([node-fetch](https://github.com/bitinn/node-fetch) when the runtime has no native `fetch` API).
the passed options and sends the request using [fetch](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API).
styfle marked this conversation as resolved.
Show resolved Hide resolved

<!-- update table of contents by running `npx markdown-toc README.md -i` -->

Expand Down Expand Up @@ -319,7 +318,7 @@ const { data: app } = await requestWithAuth(
Function
</td>
<td>
Custom replacement for <a href="https://github.com/bitinn/node-fetch">built-in fetch method</a>. Useful for testing or request hooks.
Custom replacement for <a href="https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API">fetch</a>. Useful for testing or request hooks.
</td>
</tr>
<tr>
Expand Down
18 changes: 13 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
"@octokit/request-error": "^3.0.0",
"@octokit/types": "^9.0.0",
"is-plain-object": "^5.0.0",
"node-fetch": "^2.6.7",
"universal-user-agent": "^6.0.0"
},
"devDependencies": {
Expand All @@ -44,6 +43,7 @@
"fetch-mock": "^9.3.1",
"jest": "^29.0.0",
"lolex": "^6.0.0",
"node-fetch": "^2.6.7",
"prettier": "2.8.8",
"semantic-release-plugin-update-version-in-files": "^1.0.0",
"string-to-arraybuffer": "^1.0.2",
Expand Down
9 changes: 4 additions & 5 deletions src/fetch-wrapper.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { isPlainObject } from "is-plain-object";
import nodeFetch, { HeadersInit, Response } from "node-fetch";
import { RequestError } from "@octokit/request-error";
import { EndpointInterface } from "@octokit/types";

Expand All @@ -26,10 +25,10 @@ export default function fetchWrapper(
let status: number;
let url: string;

const fetch: typeof nodeFetch =
(requestOptions.request && requestOptions.request.fetch) ||
globalThis.fetch ||
/* istanbul ignore next */ nodeFetch;
let { fetch } = globalThis;
if (requestOptions.request?.fetch) {
fetch = requestOptions.request.fetch;
}

return fetch(
wolfy1339 marked this conversation as resolved.
Show resolved Hide resolved
requestOptions.url,
Expand Down
1 change: 0 additions & 1 deletion src/get-buffer-response.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { Response } from "node-fetch";
export default function getBufferResponse(response: Response) {
return response.arrayBuffer();
}
10 changes: 7 additions & 3 deletions test/request.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
if (parseInt(process.versions.node.split(".")[0]) < 18) {
const fetch = require("node-fetch");
globalThis.fetch = fetch;
globalThis.Headers = fetch.Headers;
}
wolfy1339 marked this conversation as resolved.
Show resolved Hide resolved
import fs from "fs";
import stream from "stream";

import { getUserAgent } from "universal-user-agent";
import fetchMock from "fetch-mock";
import { Headers, RequestInit } from "node-fetch";
import { createAppAuth } from "@octokit/auth-app";
import lolex from "lolex";
import {
Expand Down Expand Up @@ -447,7 +451,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w==
});

it("passes node-fetch options to fetch only", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove timeout parameter tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it from the tests in ea7c2ab

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Removed in 3cfc7c8

const mock = (url: string, options: RequestInit) => {
const mock = (url: string, options: { timeout: number }) => {
expect(url).toEqual("https://api.github.com/");
expect(options.timeout).toEqual(100);
return Promise.reject(new Error("ok"));
Expand Down Expand Up @@ -890,7 +894,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w==
});
};

const mock = (url: string, options: RequestInit) => {
const mock = (url: string, options: { timeout: number }) => {
expect(url).toEqual("https://api.github.com/");
expect(options.timeout).toEqual(100);
return delay().then(() => {
Expand Down