Skip to content

Commit

Permalink
Enhance code to build the default Fetcher (#218)
Browse files Browse the repository at this point in the history
Fixes #215 

This PR is based on the work of @jesseditson in #216 and addresses two issues:
- We use dynamic imports to build the default `Fetcher` instance, but some JS runtimes don't support them.
- We are being too specific when detecting the presence of a `fetch` function, and different JS runtimes might expose it in different locations.

To address these issues, this PR:
- Eagerly tries to import all the built-in `Fetcher` implementations while tracking any import errors to provide actionable feedback to end-users as early as possible.
- Detects the presence of `fetch` in a looser way to avoid differences across specific JS runtimes.
  • Loading branch information
ggalmazor authored Oct 11, 2024
1 parent 2fd1b24 commit ab57d70
Show file tree
Hide file tree
Showing 32 changed files with 418 additions and 393 deletions.
29 changes: 29 additions & 0 deletions lib/fetcher/fetch-fetcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { DNSimple, TimeoutError } from "../main";
import type { Fetcher } from "./fetcher";

const fetchFetcher: Fetcher = async (params: {
method: string;
url: string;
headers: { [name: string]: string };
timeout: number;
body?: string;
}) => {
const abortController = new AbortController();
setTimeout(() => abortController.abort(), DNSimple.DEFAULT_TIMEOUT);
try {
const response = await fetch(params.url, {
method: params.method,
headers: params.headers,
body: params.body,
signal: abortController.signal,
});
return { status: response.status, body: await response.text() };
} catch (error) {
if (abortController && abortController.signal.aborted)
throw new TimeoutError();

throw error;
}
};

export default fetchFetcher;
41 changes: 41 additions & 0 deletions lib/fetcher/fetcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* A function that makes an HTTP request. It's responsible for throwing {@link TimeoutError} and aborting the request on {@param params.timeout}.
* It should return the response status and full body as a string. It should not throw on any status, even if 4xx or 5xx.
* It can decide to implement retries as appropriate. The default fetcher currently does not implement any retry strategy.
*/
export type Fetcher = (params: {
method: string;
url: string;
headers: { [name: string]: string };
body?: string;
timeout: number;
}) => Promise<{
status: number;
body: string;
}>;

let fetcherImports: {
fetchFetcher: Fetcher;
httpsFetcher: Fetcher;
};
let fetcherImportError: Error | undefined;

try {
fetcherImports = {
fetchFetcher: require("./fetch-fetcher").default,
httpsFetcher: require("./https-fetcher").default,
};
} catch (error) {
fetcherImportError = error;
}

export function getRuntimeFetcher(): Fetcher {
if (fetcherImportError)
throw new Error(
`No global \`fetch\` or \`https\` module was found. Please, provide a Fetcher implementation: ${fetcherImportError}`
);

return typeof fetch === "undefined"
? fetcherImports.httpsFetcher
: fetcherImports.fetchFetcher;
}
52 changes: 52 additions & 0 deletions lib/fetcher/https-fetcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import * as https from "https";
import { URL } from "url";
import type { Fetcher } from "./fetcher";
import { TimeoutError } from "../main";

const httpsFetcher: Fetcher = (params: {
method: string;
url: string;
headers: { [name: string]: string };
timeout: number;
body?: string;
}): Promise<{ status: number; body: string }> => {
return new Promise((resolve, reject) => {
const urlObj = new URL(params.url);
const options: https.RequestOptions = {
method: params.method,
headers: params.headers,
timeout: params.timeout,
};

const req = https.request(urlObj, options, (res) => {
const chunks: Buffer[] = [];
res
.on("data", (chunk) => chunks.push(chunk))
.on("end", () => {
const body = Buffer.concat(chunks).toString("utf-8");
resolve({
status: res.statusCode || 500, // Fallback to 500 if statusCode is undefined
body: body,
});
});
});

req.on("error", (err: { code?: string }) => {
if (err.code === "ECONNRESET") reject(new TimeoutError());
else reject(err);
});

const timeoutId = setTimeout(() => {
req.destroy();
reject(new TimeoutError());
}, params.timeout);

req.on("close", () => clearTimeout(timeoutId));

if (params.body) req.write(params.body);

req.end();
});
};

export default httpsFetcher;
77 changes: 2 additions & 75 deletions lib/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { Tlds } from "./tlds";
import { VanityNameServers } from "./vanity_name_servers";
import { Webhooks } from "./webhooks";
import { Zones } from "./zones";
import { type Fetcher, getRuntimeFetcher } from "./fetcher/fetcher";

export * from "./types";

Expand Down Expand Up @@ -102,80 +103,6 @@ export class ServerError extends RequestError {
}
}

/**
* A function that makes an HTTP request. It's responsible for throwing {@link TimeoutError} and aborting the request on {@param params.timeout}.
* It should return the response status and full body as a string. It should not throw on any status, even if 4xx or 5xx.
* It can decide to implement retries as appropriate. The default fetcher currently does not implement any retry strategy.
*/
export type Fetcher = (params: {
method: string;
url: string;
headers: { [name: string]: string };
body?: string;
// Since Node.js 14 doesn't support AbortController, we cannot simply provide the AbortSignal directly. We should move to that once we drop support for Node.js 14.
timeout: number;
}) => Promise<{
status: number;
body: string;
}>;

const getFetcherForPlatform = (): Fetcher => {
// @ts-ignore
if (typeof window == "object") {
return async ({ url, timeout, ...req }) => {
const abortController = new AbortController();
if (timeout) {
setTimeout(() => abortController.abort(), timeout);
}
try {
const res = await fetch(url, {
...req,
signal: abortController.signal,
});
const status = res.status;
const body = await res.text();
return { status, body };
} catch (err) {
// Don't just check `err.name == "AbortError"`, as that could be any AbortController and aborted for any reason. Only `abortController` signifies tiemout.
if (abortController.signal.aborted) {
throw new TimeoutError();
}
throw err;
}
};
}
const { Buffer }: typeof import("buffer") = require("buffer");
const https: typeof import("https") = require("https");
return ({ url, method, headers, timeout, body }) =>
new Promise((resolve, reject) => {
const req = https.request(url, {
method,
headers,
timeout,
});
req
.on("response", (res) => {
const chunks = Array<Buffer>();
res
.on("data", (chunk) => chunks.push(chunk))
.on("end", () =>
resolve({
status: res.statusCode!,
body: Buffer.concat(chunks).toString("utf-8"),
})
)
.on("error", reject);
})
.on("timeout", () => {
// A Promise can only be fulfilled once, so we don't need to flag this; any further "error" events with `reject` calls will do nothing.
req.destroy();
reject(new TimeoutError());
})
.on("error", reject)
.end(body);
});
};

export class DNSimple {
static VERSION = pkg.version;
static DEFAULT_TIMEOUT = 120000;
Expand Down Expand Up @@ -206,7 +133,7 @@ export class DNSimple {
constructor({
accessToken,
baseUrl = DNSimple.DEFAULT_BASE_URL,
fetcher = getFetcherForPlatform(),
fetcher = getRuntimeFetcher(),
timeout = DNSimple.DEFAULT_TIMEOUT,
userAgent = "",
}: {
Expand Down
4 changes: 2 additions & 2 deletions test/accounts.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { createTestClient, fetchMockResponse } from "./util";
import { createTestClient, responseFromFixture } from "./util";
import fetchMock from "fetch-mock";

const dnsimple = createTestClient();
Expand All @@ -8,7 +8,7 @@ describe("accounts", () => {
it("produces an account list", async () => {
fetchMock.get(
"https://api.dnsimple.com/v2/accounts",
fetchMockResponse("listAccounts/success-account.http")
responseFromFixture("listAccounts/success-account.http")
);

const result = await dnsimple.accounts.listAccounts();
Expand Down
8 changes: 4 additions & 4 deletions test/billing.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ClientError } from "../lib/main";
import { createTestClient, fetchMockResponse } from "./util";
import { createTestClient, responseFromFixture } from "./util";
import fetchMock from "fetch-mock";

const dnsimple = createTestClient();
Expand All @@ -11,7 +11,7 @@ describe("billing", () => {
it("produces a charges list", async () => {
fetchMock.get(
"https://api.dnsimple.com/v2/1010/billing/charges",
fetchMockResponse("listCharges/success.http")
responseFromFixture("listCharges/success.http")
);

const response = await dnsimple.billing.listCharges(accountId);
Expand Down Expand Up @@ -79,7 +79,7 @@ describe("billing", () => {
it("throws an error on bad filter", async () => {
fetchMock.get(
"https://api.dnsimple.com/v2/1010/billing/charges",
fetchMockResponse("listCharges/fail-400-bad-filter.http")
responseFromFixture("listCharges/fail-400-bad-filter.http")
);

try {
Expand All @@ -97,7 +97,7 @@ describe("billing", () => {
it("throws an error on missing scope", async () => {
fetchMock.get(
"https://api.dnsimple.com/v2/1010/billing/charges",
fetchMockResponse("listCharges/fail-403.http")
responseFromFixture("listCharges/fail-403.http")
);

try {
Expand Down
Loading

0 comments on commit ab57d70

Please sign in to comment.