src: simplify router, add cache middleware tests (#713)

Signed-off-by: flakey5 <[email protected]>

Author: flakey5

src/context.ts

@@ -4,5 +4,5 @@ import type { Env } from './env';
 export interface Context {
   env: Env;
   execution: ExecutionContext;
-  sentry: Toucan;
+  sentry?: Toucan;
 }

src/middleware/cacheMiddleware.test.ts

@@ -0,0 +1,158 @@
+import { expect, test, vi } from 'vitest';
+import { cached } from './cacheMiddleware';
+import type { Context } from '../context';
+import type { Middleware } from './middleware';
+
+class TestMiddleware implements Middleware {
+  #successful = true;
+
+  constructor(successful = true) {
+    this.#successful = successful;
+  }
+
+  async handle(): Promise<Response> {
+    return new Response('asd', { status: this.#successful ? 200 : 500 });
+  }
+}
+
+test('bypasses cache when caching is disabled', async () => {
+  const ctx: Context = {
+    // @ts-expect-error don't care
+    env: {
+      CACHING: false,
+    },
+    // @ts-expect-error don't care
+    execution: {
+      waitUntil: (): void => {
+        throw new Error('should not hit this');
+      },
+    },
+  };
+
+  const middleware = cached(new TestMiddleware());
+
+  // @ts-expect-error don't need WorkerRequest instance here
+  await middleware.handle(new Request('https://localhost'), ctx);
+});
+
+test('returns cached response', async () => {
+  const request = new Request('https://localhost');
+  const cachedResponse = new Response('cached');
+
+  vi.stubGlobal(
+    'caches',
+    // eslint-disable-next-line @typescript-eslint/explicit-function-return-type
+    (() => ({
+      open: vi.fn(() => ({
+        match(requestToMatch: Request): Promise<Response | undefined> {
+          expect(requestToMatch).toStrictEqual(request);
+          return Promise.resolve(cachedResponse);
+        },
+        put(): Promise<void> {
+          throw new Error('should not hit this');
+        },
+      })),
+    }))()
+  );
+
+  const ctx: Context = {
+    // @ts-expect-error don't care
+    env: {
+      CACHING: true,
+    },
+  };
+
+  try {
+    const middleware = cached(new TestMiddleware());
+
+    // @ts-expect-error don't need WorkerRequest instance here
+    const res = await middleware.handle(new Request('https://localhost'), ctx);
+    expect(res).toStrictEqual(cachedResponse);
+  } finally {
+    vi.unstubAllGlobals();
+  }
+});
+
+test('caches successful response', async () => {
+  let responseCached = false;
+  const request = new Request('https://localhost');
+
+  vi.stubGlobal(
+    'caches',
+    // eslint-disable-next-line @typescript-eslint/explicit-function-return-type
+    (() => ({
+      open: vi.fn(() => ({
+        match(): Promise<Response | undefined> {
+          return Promise.resolve(undefined);
+        },
+        put(requestToCache: Request, _: Response): Promise<void> {
+          responseCached = true;
+          expect(requestToCache).toStrictEqual(request);
+
+          return Promise.resolve();
+        },
+      })),
+    }))()
+  );
+
+  const ctx: Context = {
+    // @ts-expect-error don't care
+    env: {
+      CACHING: true,
+    },
+    // @ts-expect-error don't care
+    execution: {
+      waitUntil(_: Promise<unknown>): void {},
+    },
+  };
+
+  try {
+    const middleware = cached(new TestMiddleware());
+
+    // @ts-expect-error don't need WorkerRequest instance here
+    const res = await middleware.handle(request, ctx);
+    expect(await res.text()).toStrictEqual('asd');
+
+    expect(responseCached).toBeTruthy();
+  } finally {
+    vi.unstubAllGlobals();
+  }
+});
+
+test('does not cache non-200 response', async () => {
+  vi.stubGlobal(
+    'caches',
+    // eslint-disable-next-line @typescript-eslint/explicit-function-return-type
+    (() => ({
+      open: vi.fn(() => ({
+        match(): Promise<Response | undefined> {
+          return Promise.resolve(undefined);
+        },
+        put(): Promise<void> {
+          throw new Error('should not hit this');
+        },
+      })),
+    }))()
+  );
+
+  const ctx: Context = {
+    // @ts-expect-error don't care
+    env: {
+      CACHING: true,
+    },
+    // @ts-expect-error don't care
+    execution: {
+      waitUntil(_: Promise<unknown>): void {},
+    },
+  };
+
+  try {
+    const middleware = cached(new TestMiddleware(false));
+
+    // @ts-expect-error don't need WorkerRequest instance here
+    const res = await middleware.handle(new Request('https://localhost'), ctx);
+    expect(await res.text()).toStrictEqual('asd');
+  } finally {
+    vi.unstubAllGlobals();
+  }
+});

src/middleware/cacheMiddleware.ts

@@ -21,39 +21,30 @@ export function cached(middleware: Middleware): Middleware {
   let cache: Cache;
 
   const wrapper: Middleware = {
-    async handle(request, ctx, next) {
-      ctx.sentry.addBreadcrumb({
+    async handle(request, ctx) {
+      ctx.sentry?.addBreadcrumb({
         category: 'CacheMiddleware',
         data: {
           underlyingMiddleware: middleware.constructor.name,
         },
       });
 
+      // Caching globally disabled, skip
       if (!ctx.env.CACHING) {
-        return middleware.handle(request, ctx, next);
+        return middleware.handle(request, ctx);
       }
 
-      if (cache === undefined) {
-        cache = await caches.open(middleware.constructor.name);
-      }
+      cache ??= await caches.open(middleware.constructor.name);
 
       // Check if the request is in the cache already, return it if so
       let response = await cache.match(request);
       if (response !== undefined) {
         return response;
       }
 
-      // Set to true when the middleware this wraps calls next().
-      //  We only want to cache the result for this middleware,
-      //  not the one after this.
-      let wasDeferred = false;
-
-      response = await middleware.handle(request, ctx, () => {
-        wasDeferred = true;
-        return next();
-      });
+      response = await middleware.handle(request, ctx);
 
-      if (!wasDeferred && response.status === 200) {
+      if (response instanceof Response && response.status === 200) {
         // Successful request, let's cache it for next time
         const cachedResponse = response.clone();
 

src/middleware/middleware.ts

@@ -9,9 +9,5 @@ export interface Middleware {
    * @param next Calls the next middleware in the chain. This may also call
    *  other middlewares before returning.
    */
-  handle(
-    request: Request,
-    ctx: Context,
-    next: MiddlewareNext
-  ): Promise<Response>;
+  handle(request: Request, ctx: Context): Promise<Response>;
 }

src/middleware/substituteMiddleware.test.ts

@@ -1,7 +1,7 @@
 import { describe, test, expect } from 'vitest';
 import { SubtitutionMiddleware } from './subtituteMiddleware';
 import type { Request as WorkerRequest } from '../routes/request';
-import type { Router } from '../routes';
+import type { Router } from '../routes/router';
 
 describe('SubtituteMiddleware', () => {
   test('correctly substitutes url `/dist/latest` to `/dist/v1.0.0`', () => {
@@ -11,7 +11,7 @@ describe('SubtituteMiddleware', () => {
     originalRequest.urlObj = new URL(originalUrl);
 
     const router: Partial<Router> = {
-      handle: (substitutedRequest: WorkerRequest, _, unsubstitutedUrl) => {
+      fetch: (substitutedRequest: WorkerRequest, _, unsubstitutedUrl) => {
         // Has the url been substituted? (latest -> v1.0.0)
         // strictEqual(substitutedRequest.url, 'https://localhost/dist/v1.0.0');
         expect(substitutedRequest.url).toStrictEqual(

src/middleware/subtituteMiddleware.ts

@@ -1,5 +1,5 @@
 import type { Context } from '../context';
-import type { Router } from '../routes';
+import type { Router } from '../routes/router';
 import type { Request } from '../routes/request';
 import type { Middleware } from './middleware';
 
@@ -25,7 +25,7 @@ export class SubtitutionMiddleware implements Middleware {
   handle(request: Request, ctx: Context): Promise<Response> {
     const newUrl = request.url.replaceAll(this.searchValue, this.replaceValue);
 
-    ctx.sentry.addBreadcrumb({
+    ctx.sentry?.addBreadcrumb({
       type: 'navigation',
       category: 'SubstitutionMiddleware',
       data: {
@@ -39,6 +39,6 @@ export class SubtitutionMiddleware implements Middleware {
       new Request(request)
     );
 
-    return this.router.handle(substitutedRequest, ctx, request.urlObj);
+    return this.router.fetch(substitutedRequest, ctx, request.urlObj);
   }
 }

src/routes/index.ts

@@ -19,13 +19,13 @@ export function registerRoutes(router: Router): void {
     'https://github.com/nodejs/corepack#readme'
   );
 
-  router.options('*', [new OptionsMiddleware()]);
+  router.options('*', new OptionsMiddleware());
 
-  router.head('/metrics/?:filePath+', [r2Middleware, originMiddleware]);
-  router.get('/metrics/?:filePath+', [cachedR2Middleware, originMiddleware]);
+  router.head('/metrics/?:filePath+', r2Middleware, originMiddleware);
+  router.get('/metrics/?:filePath+', cachedR2Middleware, originMiddleware);
 
-  router.all('/api/corepack.html', [corepackRedirectMiddleware]);
-  router.all('/docs/latest/api/corepack.html', [corepackRedirectMiddleware]);
+  router.all('/api/corepack.html', corepackRedirectMiddleware);
+  router.all('/docs/latest/api/corepack.html', corepackRedirectMiddleware);
 
   // Register routes for latest releases (e.g. `/dist/latest/`)
   for (const branch in latestVersions) {
@@ -36,39 +36,40 @@ export function registerRoutes(router: Router): void {
       latestVersion
     );
 
-    router.head(`/dist/${branch}*`, [subtitutionMiddleware]);
-    router.get(`/dist/${branch}*`, [subtitutionMiddleware]);
+    router.head(`/dist/${branch}*`, subtitutionMiddleware);
+    router.get(`/dist/${branch}*`, subtitutionMiddleware);
 
-    router.head(`/download/release/${branch}*`, [subtitutionMiddleware]);
-    router.get(`/download/release/${branch}*`, [subtitutionMiddleware]);
+    router.head(`/download/release/${branch}*`, subtitutionMiddleware);
+    router.get(`/download/release/${branch}*`, subtitutionMiddleware);
 
-    router.head(`/docs/${branch}*`, [subtitutionMiddleware]);
-    router.get(`/docs/${branch}*`, [subtitutionMiddleware]);
+    router.head(`/docs/${branch}*`, subtitutionMiddleware);
+    router.get(`/docs/${branch}*`, subtitutionMiddleware);
   }
 
-  router.head('/node-config-schema.json', [r2Middleware]);
-  router.get('/node-config-schema.json', [r2Middleware]);
+  router.head('/node-config-schema.json', r2Middleware);
+  router.get('/node-config-schema.json', r2Middleware);
 
-  router.head('/dist/?:filePath+', [r2Middleware, originMiddleware]);
-  router.get('/dist/?:filePath+', [cachedR2Middleware, originMiddleware]);
+  router.head('/dist/?:filePath+', r2Middleware, originMiddleware);
+  router.get('/dist/?:filePath+', cachedR2Middleware, originMiddleware);
 
-  router.head('/download/?:filePath+', [r2Middleware, originMiddleware]);
-  router.get('/download/?:filePath+', [cachedR2Middleware, originMiddleware]);
+  router.head('/download/?:filePath+', r2Middleware, originMiddleware);
+  router.get('/download/?:filePath+', cachedR2Middleware, originMiddleware);
 
-  router.head('/api/?:filePath+', [r2Middleware, originMiddleware]);
-  router.get('/api/?:filePath+', [cachedR2Middleware, originMiddleware]);
+  router.head('/api/?:filePath+', r2Middleware, originMiddleware);
+  router.get('/api/?:filePath+', cachedR2Middleware, originMiddleware);
 
-  router.head('/docs/?:version?/:filePath+?', [r2Middleware, originMiddleware]);
-  router.get('/docs/?:version?/:filePath+?', [
+  router.head('/docs/?:version?/:filePath+?', r2Middleware, originMiddleware);
+  router.get(
+    '/docs/?:version?/:filePath+?',
     cachedR2Middleware,
-    originMiddleware,
-  ]);
+    originMiddleware
+  );
 
-  router.post('/_throw', [new ThrowMiddleware()]);
+  router.post('/_throw', new ThrowMiddleware());
 
-  router.get('*', [new NotFoundMiddleware()]);
+  router.get('*', new NotFoundMiddleware());
 
-  router.all('*', [new MethodNotAllowedMiddleware()]);
+  router.all('*', new MethodNotAllowedMiddleware());
 }
 
 export * from './router';