fix: add retry logic for fetches, fix small data:// bug in fetches (#216

)
jspm · Feb 21, 2023 · ba62529 · ba62529
1 parent dea9d09
commit ba62529
Show file tree

Hide file tree

Showing 16 changed files with 242 additions and 181 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -58,6 +58,7 @@
     "lit-element": "^2.5.1",
     "mocha": "^9.2.2",
     "open": "^8.4.1",
+    "prettier": "^2.8.4",
     "rollup": "^2.79.1",
     "typedoc": "^0.22.18"
   },

diff --git a/src/common/fetch-common.ts b/src/common/fetch-common.ts
@@ -0,0 +1,29 @@
+export interface Response {
+  status: number;
+  statusText?: string;
+  text?(): Promise<string>;
+  json?(): Promise<any>;
+  arrayBuffer?(): ArrayBuffer;
+}
+
+export type FetchFn = (
+  url: URL,
+  ...args: any[]
+) => Promise<Response | globalThis.Response>;
+
+/**
+ * Wraps a fetch request with retry logic on exceptions, which is useful for
+ * spotty connections that may fail intermittently.
+ */
+export function wrapWithRetry(fetch: FetchFn): FetchFn {
+  return async function (url: URL, ...args: any[]) {
+    let retries = 0;
+    while (true) {
+      try {
+        return await fetch(url, ...args);
+      } catch (e) {
+        if (retries++ > 3) throw e;
+      }
+    }
+  };
+}
diff --git a/src/common/fetch-deno.ts b/src/common/fetch-deno.ts
@@ -1,12 +1,12 @@
-// @ts-ignore
 import { fileURLToPath } from "url";
-// @ts-ignore
-// Caching disabled due to not respecting cache headers...
-// import { cache } from "https://deno.land/x/cache/mod.ts";
+import { wrapWithRetry, FetchFn } from "./fetch-common.js";
 
 export function clearCache() {}
 
-export const fetch = async function (url: URL, ...args: any[]) {
+export const fetch: FetchFn = wrapWithRetry(async function (
+  url: URL,
+  ...args: any[]
+) {
   const urlString = url.toString();
   if (
     urlString.startsWith("file:") ||
@@ -16,12 +16,14 @@ export const fetch = async function (url: URL, ...args: any[]) {
     try {
       let source: string;
       if (urlString.startsWith("file:")) {
-        // @ts-ignore
-        source = await Deno.readTextFile(fileURLToPath(urlString));
+        // @ts-ignore - can only resolve Deno when running in Deno
+        source = (await Deno.readTextFile(fileURLToPath(urlString))) as string;
       } else if (urlString.startsWith("node:")) {
         source = "";
       } else {
-        source = decodeURIComponent(urlString.slice(urlString.indexOf(",")));
+        source = decodeURIComponent(
+          urlString.slice(urlString.indexOf(",") + 1)
+        );
       }
       return {
         status: 200,
@@ -32,7 +34,7 @@ export const fetch = async function (url: URL, ...args: any[]) {
           return JSON.parse(source.toString());
         },
         arrayBuffer() {
-          return source;
+          return new TextEncoder().encode(source.toString()).buffer;
         },
       };
     } catch (e) {
@@ -55,34 +57,5 @@ export const fetch = async function (url: URL, ...args: any[]) {
     }
   } else {
     return globalThis.fetch(urlString, ...args);
-    // let file;
-    // try {
-    //   file = await cache(urlString);
-    // }
-    // catch (e) {
-    //   if (e.name === 'SyntaxError') {
-    //     // Weird bug in Deno cache...
-    //     // @ts-ignore
-    //     return _fetch(url, ...args);
-    //   }
-    //   if (e.name === 'CacheError' && e.message === 'Not Found') {
-    //     return { status: 404, statusText: e.toString() };
-    //   }
-    //   throw e;
-    // }
-    // @ts-ignore
-    // const source = await Deno.readTextFile(fromFileUrl(urlString));
-    // return {
-    //   status: 200,
-    //   async text () {
-    //     return source.toString();
-    //   },
-    //   async json () {
-    //     return JSON.parse(source.toString());
-    //   },
-    //   arrayBuffer () {
-    //     return source;
-    //   }
-    // };
   }
-};
+});
diff --git a/src/common/fetch-native.ts b/src/common/fetch-native.ts
@@ -1,3 +1,5 @@
+import { FetchFn, wrapWithRetry } from "./fetch-common.js";
+
 // Browser native fetch doesn't deal well with high contention
 // restrict in-flight fetches to a pool of 100
 let p = [];
@@ -10,7 +12,7 @@ function popFetchPool() {
   if (p.length) p.shift()();
 }
 
-export async function fetch(url, opts) {
+export const fetch: FetchFn = wrapWithRetry(async function fetch(url, opts) {
   const poolQueue = pushFetchPool();
   if (poolQueue) await poolQueue;
   try {
@@ -35,5 +37,6 @@ export async function fetch(url, opts) {
   } finally {
     popFetchPool();
   }
-}
+});
+
 export const clearCache = () => {};
diff --git a/src/common/fetch-node.ts b/src/common/fetch-node.ts
@@ -1,18 +1,12 @@
 // @ts-ignore
 import version from "../version.js";
-// @ts-ignore
+import { wrapWithRetry, FetchFn } from "./fetch-common.js";
 import path from "path";
-// @ts-ignore
 import { homedir } from "os";
-// @ts-ignore
 import process from "process";
-// @ts-ignore
 import rimraf from "rimraf";
-// @ts-ignore
 import makeFetchHappen from "make-fetch-happen";
-// @ts-ignore
 import { readFileSync } from "fs";
-// @ts-ignore
 import { Buffer } from "buffer";
 
 let cacheDir: string;
@@ -38,7 +32,7 @@ const _fetch = makeFetchHappen.defaults({
   headers: { "User-Agent": `jspm/generator@${version}` },
 });
 
-function sourceResponse(buffer) {
+function sourceResponse(buffer: string | Buffer) {
   return {
     status: 200,
     async text() {
@@ -48,7 +42,8 @@ function sourceResponse(buffer) {
       return JSON.parse(buffer.toString());
     },
     arrayBuffer() {
-      return buffer.buffer || buffer;
+      if (buffer instanceof Buffer) return buffer.buffer;
+      return new TextEncoder().encode(buffer.toString()).buffer;
     },
   };
 }
@@ -66,7 +61,10 @@ const dirResponse = {
   },
 };
 
-export const fetch = async function (url: URL, opts?: Record<string, any>) {
+export const fetch: FetchFn = wrapWithRetry(async function (
+  url: URL,
+  opts?: Record<string, any>
+) {
   if (!opts) throw new Error("Always expect fetch options to be passed");
   const urlString = url.toString();
   const protocol = urlString.slice(0, urlString.indexOf(":") + 1);
@@ -98,7 +96,7 @@ export const fetch = async function (url: URL, opts?: Record<string, any>) {
       }
     case "data:":
       return sourceResponse(
-        decodeURIComponent(urlString.slice(urlString.indexOf(",")))
+        decodeURIComponent(urlString.slice(urlString.indexOf(",") + 1))
       );
     case "node:":
       return sourceResponse("");
@@ -107,4 +105,4 @@ export const fetch = async function (url: URL, opts?: Record<string, any>) {
       // @ts-ignore
       return _fetch(url, opts);
   }
-};
+});
diff --git a/src/common/fetch-vscode.ts b/src/common/fetch-vscode.ts
@@ -1,3 +1,4 @@
+import { FetchFn, wrapWithRetry } from "./fetch-common.js";
 import { fetch as _fetch } from "./fetch-native.js";
 export { clearCache } from "./fetch-native.js";
 
@@ -32,7 +33,10 @@ const dirResponse = {
 // @ts-ignore
 const vscode = require("vscode");
 
-export const fetch = async function (url: URL, opts?: Record<string, any>) {
+export const fetch: FetchFn = wrapWithRetry(async function (
+  url: URL,
+  opts?: Record<string, any>
+) {
   if (!opts) throw new Error("Always expect fetch options to be passed");
   const urlString = url.toString();
   const protocol = urlString.slice(0, urlString.indexOf(":") + 1);
@@ -71,4 +75,4 @@ export const fetch = async function (url: URL, opts?: Record<string, any>) {
       // @ts-ignore
       return _fetch(url, opts);
   }
-};
+});
diff --git a/src/install/installer.ts b/src/install/installer.ts
@@ -206,7 +206,10 @@ export class Installer {
     }
 
     if (pkgTarget instanceof URL) {
-      this.log("installer/installTarget", `${pkgName} ${pkgScope} -> ${pkgTarget.href} (URL)`);
+      this.log(
+        "installer/installTarget",
+        `${pkgName} ${pkgScope} -> ${pkgTarget.href} (URL)`
+      );
       const installUrl = (pkgTarget.href +
         (pkgTarget.href.endsWith("/") ? "" : "/")) as `${string}/`;
       this.newInstalls = setResolution(
@@ -267,7 +270,9 @@ export class Installer {
         if (pkg) {
           this.log(
             "installer/installTarget",
-            `${pkgName} ${pkgScope} -> ${JSON.stringify(latestPkg)} (existing match not latest)`
+            `${pkgName} ${pkgScope} -> ${JSON.stringify(
+              latestPkg
+            )} (existing match not latest)`
           );
           const installUrl = this.resolver.pkgToUrl(pkg, provider);
           this.newInstalls = setResolution(
@@ -318,7 +323,10 @@ export class Installer {
     traceSubpath: `./${string}` | ".",
     parentUrl: string = this.installBaseUrl
   ): Promise<string | InstalledResolution> {
-    this.log("installer/install", `installing ${pkgName} from ${parentUrl} in scope ${pkgScope}`);
+    this.log(
+      "installer/install",
+      `installing ${pkgName} from ${parentUrl} in scope ${pkgScope}`
+    );
     if (!this.installing) throwInternalError("Not installing");
 
     // Anything installed in the scope of the installer's base URL is treated
@@ -348,7 +356,12 @@ export class Installer {
       isTopLevel ? null : pkgScope
     );
     if (existingResolution) {
-      this.log("installer/install", `existing lock for ${pkgName} from ${parentUrl} in scope ${pkgScope} is ${JSON.stringify(existingResolution)}`);
+      this.log(
+        "installer/install",
+        `existing lock for ${pkgName} from ${parentUrl} in scope ${pkgScope} is ${JSON.stringify(
+          existingResolution
+        )}`
+      );
       return existingResolution;
     }
 

diff --git a/src/install/lock.ts b/src/install/lock.ts
@@ -36,14 +36,14 @@ export interface LockResolutions {
 }
 
 interface PackageTargetMap {
-  [pkgName: string]: PackageTarget | URL,
+  [pkgName: string]: PackageTarget | URL;
 }
 
 export interface VersionConstraints {
-  primary: PackageTargetMap,
+  primary: PackageTargetMap;
   secondary: {
-    [pkgUrl: `${string}/`]: PackageTargetMap,
-  }
+    [pkgUrl: `${string}/`]: PackageTargetMap;
+  };
 }
 
 export interface InstalledResolution {
@@ -321,14 +321,22 @@ export function getInstallsFor(
   const installs: PackageInstall[] = [];
   for (const alias of Object.keys(constraints.primary)) {
     const target = constraints.primary[alias];
-    if (!(target instanceof URL) && target.registry === registry && target.name === name)
+    if (
+      !(target instanceof URL) &&
+      target.registry === registry &&
+      target.name === name
+    )
       installs.push({ alias, pkgScope: null, ranges: target.ranges });
   }
   for (const pkgScope of Object.keys(constraints.secondary) as `${string}/`[]) {
     const scope = constraints.secondary[pkgScope];
     for (const alias of Object.keys(scope)) {
       const target = scope[alias];
-      if (!(target instanceof URL) && target.registry === registry && target.name === name)
+      if (
+        !(target instanceof URL) &&
+        target.registry === registry &&
+        target.name === name
+      )
         installs.push({ alias, pkgScope, ranges: target.ranges });
     }
   }

diff --git a/src/install/package.ts b/src/install/package.ts
@@ -211,7 +211,8 @@ export async function parseTarget(
   // package config if they exist:
   const pcfg = await resolver.getPackageConfig(parentPkgUrl.href);
   if (versionIndex === -1 && pcfg) {
-    const dep = pcfg.dependencies?.[alias] ||
+    const dep =
+      pcfg.dependencies?.[alias] ||
       pcfg.peerDependencies?.[alias] ||
       pcfg.optionalDependencies?.[alias] ||
       pcfg.devDependencies?.[alias];
@@ -226,7 +227,7 @@ export async function parseTarget(
         ),
         alias,
         subpath: pkg.subpath,
-      }
+      };
     }
   }
 

diff --git a/src/trace/resolver.ts b/src/trace/resolver.ts
@@ -95,9 +95,7 @@ export class Resolver {
     this.providers = Object.assign({}, this.providers, { [name]: provider });
   }
 
-  parseUrlPkg(
-    url: string
-  ): {
+  parseUrlPkg(url: string): {
     pkg: ExactPackage;
     subpath: null | `./${string}`;
     source: { layer: string; provider: string };