Add more comments around workerd usage (#2575)

* Add clarifications around MiniOxygen and workerd

* More comments

packages/mini-oxygen/src/worker/assets.ts

@@ -21,6 +21,10 @@ export function buildAssetsUrl(assetsPort: number) {
 /**
  * Creates a server that serves static assets from the build directory.
  * Mimics Shopify CDN URLs for Oxygen v2.
+ * Note: this is not used when running with Vite because it already
+ * serves transformed assets before reaching MiniOxygen.
+ * See the following for more details:
+ * https://github.com/Shopify/hydrogen/pull/2078#issuecomment-2121705993
  */
 export function createAssetsServer(assetsDirectory: string) {
   return createServer(async (req: IncomingMessage, res: ServerResponse) => {

packages/mini-oxygen/src/worker/devtools.ts

@@ -20,6 +20,14 @@ const FAVICON_URL =
 
 export type InspectorProxy = ReturnType<typeof createInspectorProxy>;
 
+/**
+ * Creates a proxy server that forwards messages between the local
+ * debugger (e.g. VSCode, Browser DevTools) and the Workerd inspector.
+ * It also serves a custom in-browser DevTools UI for MiniOxygen by
+ * proxying the Cloudflare DevTools (used in Wrangler / Miniflare),
+ * and fixes a few issues related to serving this tool locally.
+ *
+ */
 export function createInspectorProxy(
   port: number,
   newInspectorConnection: InspectorConnection,
@@ -45,13 +53,15 @@ export function createInspectorProxy(
   const sourceMapPathname = '/__index.js.map';
   const sourceMapURL = `http://localhost:${port}${sourceMapPathname}`;
 
+  // Create the proxy server used when running with `--debug` flag:
   const server = createServer((req: IncomingMessage, res: ServerResponse) => {
     // Remove query params. E.g. `/json/list?for_tab`
     const [url = '/', queryString = ''] = req.url?.split('?') || [];
 
     switch (url) {
-      // We implement a couple of well known end points
-      // that are queried for metadata by chrome://inspect
+      // We implement a couple of well known end points that are queried
+      // for metadata when opening `chrome://inspect` in the browser.
+      // https://chromedevtools.github.io/devtools-protocol/#endpoints
       case '/json/version':
         res.setHeader('Content-Type', 'application/json');
         res.end(
@@ -84,7 +94,9 @@ export function createInspectorProxy(
         }
         return;
       case sourceMapPathname:
-        // Handle proxied sourcemaps
+        // Handle proxied sourcemaps. This is only used when serving
+        // a built application in h2:preview or classic project dev.
+        // h2:dev with Vite uses inlined sourcemaps instead.
         res.setHeader('Content-Type', 'text/plain');
         res.setHeader('Cache-Control', 'no-store');
         res.setHeader(
@@ -100,6 +112,7 @@ export function createInspectorProxy(
         }
         break;
       case '/favicon.ico':
+        // The browser requests for this automatically when opening DevTools.
         proxyHttp(FAVICON_URL, req.headers, res);
         break;
       case '/':
@@ -112,7 +125,7 @@ export function createInspectorProxy(
           );
           res.end();
         } else {
-          // Proxy CFW DevTools UI.
+          // Proxy the main page of the original CFW DevTools UI.
           proxyHttp(
             CFW_DEVTOOLS + '/js_app',
             req.headers,
@@ -128,6 +141,7 @@ export function createInspectorProxy(
         }
         break;
       default:
+        // Proxy assets from the original CFW DevTools UI, modifying them as needed.
         if (
           url === '/panels/sources/sources-meta.js' ||
           (url.startsWith('/core/i18n/locales/') && url.endsWith('.json'))
@@ -157,7 +171,8 @@ export function createInspectorProxy(
 
   wsServer.on('connection', (ws, req) => {
     if (wsServer.clients.size > 1) {
-      // Only support one active Devtools instance at a time.
+      // Only support one active DevTools instance at a time. E.g.
+      // either VSCode/editor debugger or 1 browser DevTools tab.
       console.error(
         'Tried to open a new devtools window when a previous one was already open.',
       );
@@ -190,12 +205,22 @@ export function createInspectorProxy(
 
   if (inspector.ws) onInspectorConnection();
 
+  /**
+   * This function is called when the inspector connection is established
+   * for the first time or when the inspector is reconnected. That happens
+   * when the source code is reloaded in h2:preview, h2:debug:cpu.
+   * However, it no longer happens in h2:dev with Vite because the worker
+   * instance is not reloaded after source code changes, only patched with HMR.
+   */
   function onInspectorConnection() {
     inspector.ws.addEventListener('message', sendMessageToDebugger);
 
     // In case this is a DevTools connection, send a warning
     // message to the console to inform about reconnection.
     // VSCode can reconnect automatically with `restart: true`.
+    //  > TODO: it would be good to send this message also in h2:dev with Vite.
+    //  > However, that requires a completely different type of wiring:
+    //  > Getting Vite's HMR notifications from this part of the code somehow.
     debuggerWs?.send(
       JSON.stringify({
         method: 'Runtime.consoleAPICalled',
@@ -234,6 +259,8 @@ export function createInspectorProxy(
   }
 
   return {
+    // Every time workerd is restarted (e.g. env var change, etc.),
+    // the inspector connection needs to be re-established.
     updateInspectorConnection(newConnection: InspectorConnection) {
       inspector = newConnection;
       onInspectorConnection();

packages/mini-oxygen/src/worker/handler.ts

@@ -12,7 +12,14 @@ export function getMiniOxygenHandlerScript() {
   return `export default { fetch: ${miniOxygenHandler} }\n${withRequestHook}`;
 }
 
-// This function is stringified, do not use anything from outer scope here:
+/**
+ * Main entry point for the worker. It serves as a router, dispatching requests
+ * to the appropriate handlers, but also to handle common cases like static assets
+ * and polyfills Oxygen headers.
+ *
+ * Since this function is stringified and executed in the "worker", do not
+ * add anything from the outer scope here.
+ */
 async function miniOxygenHandler(
   request: Request,
   env: MiniOxygenHandlerEnv,

packages/mini-oxygen/src/worker/index.ts

@@ -67,15 +67,41 @@ type GetNewOptions = (
 ) => ReloadableOptions | Promise<ReloadableOptions>;
 
 export type MiniOxygenOptions = InputMiniflareOptions & {
+  /**
+   * Allows attaching a debugger to the worker instance.
+   * @default false
+   */
   debug?: boolean;
+  /**
+   * Path to the source map file to use in debuggers, if needed.
+   * @default undefined
+   */
   sourceMapPath?: string;
+  /**
+   * Allows serving static assets from a directory or another origin.
+   * @default undefined
+   */
   assets?: AssetOptions;
+  /**
+   * Hook into requests and responses. Useful for debugging and logging.
+   * @default undefined
+   */
   requestHook?: RequestHook | null;
+  /**
+   * Name of the worker used for attaching a debugger.
+   * @default undefined The first worker in the array is used.
+   */
   inspectWorkerName?: string;
 };
 
 export type MiniOxygenInstance = ReturnType<typeof createMiniOxygen>;
 
+/**
+ * Creates a MiniOxygen instance using the Workers runtime (workerd).
+ *
+ * @param options - Options for the MiniOxygen instance.
+ * @returns A MiniOxygen instance.
+ */
 export function createMiniOxygen({
   debug = false,
   inspectorPort,

packages/mini-oxygen/src/worker/inspector.ts

@@ -46,6 +46,20 @@ export interface ErrorProperties {
   stack?: string;
 }
 
+/**
+ * Creates a connection to the workerd inspector.
+ *
+ * The messages are sent via WebSockets following the Chrome DevTools Protocol:
+ * https://chromedevtools.github.io/devtools-protocol/
+ *
+ * The inspector connection has two purposes:
+ * 1. Attach debuggers to the workerd instance.
+ * 2. Ingest logs (e.g. user's `console.log` calls) from workerd into
+ *    the main Node.js process, so that we can display them in the terminal.
+ *
+ * @param options - Options for the inspector.
+ * @returns A function to reconnect to the inspector.
+ */
 export function createInspectorConnector(options: {
   privateInspectorPort: number;
   publicInspectorPort?: number;
@@ -86,9 +100,16 @@ export function createInspectorConnector(options: {
   };
 }
 
+/**
+ * Since a workerd instance can have multiple workers, we need to find the
+ * inspector URL for the main worker that runs user code, since that's the
+ * worker we want to debug. We use the port number to query all the existing
+ * workers and find the one that matches the user worker name.
+ */
 async function findInspectorUrl(inspectorPort: number, workerName: string) {
   try {
     // Fetch the inspector JSON response from the DevTools Inspector protocol
+    // https://chromedevtools.github.io/devtools-protocol/#endpoints
     const jsonUrl = `http://127.0.0.1:${inspectorPort}/json`;
     const body = (await (
       await fetch(jsonUrl)

packages/mini-oxygen/src/worker/logger.ts

@@ -7,6 +7,14 @@ import type {
   MessageData,
 } from './inspector.js';
 
+/**
+ * Adds event listeners for console messages and exceptions to the inspector connection.
+ * Then, it handles logs and errors in the main Node.js process to display them in the terminal.
+ * It also formats and displays source maps for errors using information that only exists
+ * in the Node.js process, although this is not used for Vite processes because Vite already
+ * provides source maps for errors.
+ * @param inspector
+ */
 export function addInspectorConsoleLogger(inspector: InspectorConnection) {
   inspector.ws.addEventListener('message', async (event) => {
     if (typeof event.data !== 'string') {
@@ -28,6 +36,12 @@ export function addInspectorConsoleLogger(inspector: InspectorConnection) {
   });
 }
 
+/**
+ * Creates an Error instance in the Node.js process from an unhandled exception in workerd.
+ * @param exceptionDetails
+ * @param inspector
+ * @returns Resolves to an actual Error instance with stack trace and message.
+ */
 export async function createErrorFromException(
   exceptionDetails: Protocol.Runtime.ExceptionDetails,
   inspector: InspectorConnection,
@@ -56,6 +70,12 @@ export async function createErrorFromException(
   );
 }
 
+/**
+ * Creates an Error instance in the Node.js process from a logged error in workerd.
+ * @param ro RemoteObject representing the error logged.
+ * @param inspector
+ * @returns Resolves to an actual Error instance with stack trace and message.
+ */
 export async function createErrorFromLog(
   ro: Protocol.Runtime.RemoteObject,
   inspector: InspectorConnection,
@@ -85,14 +105,6 @@ export async function createErrorFromLog(
   return inspector.reconstructError(errorProperties, ro);
 }
 
-/**
- * This function converts a message serialised as a devtools event
- * into arguments suitable to be called by a console method, and
- * then actually calls the method with those arguments. Effectively,
- * we're just doing a little bit of the work of the devtools console,
- * directly in the terminal.
- */
-
 const mapConsoleAPIMessageTypeToConsoleMethod: {
   [key in Protocol.Runtime.ConsoleAPICalledEvent['type']]: Exclude<
     keyof Console,
@@ -119,6 +131,17 @@ const mapConsoleAPIMessageTypeToConsoleMethod: {
   endGroup: 'groupEnd',
 };
 
+/**
+ * This function converts a message serialised as a devtools event
+ * into arguments suitable to be called by a console method, and
+ * then actually calls the method with those arguments. Effectively,
+ * we're just doing a little bit of the work of the devtools console,
+ * directly in the terminal.
+ *
+ * Here we decide how to display each type of argument. For example,
+ * for Errors we reconstruct the stack trace; for Maps, we display
+ * the key-value pairs, etc.
+ */
 async function logConsoleMessage(
   evt: Protocol.Runtime.ConsoleAPICalledEvent,
   inspector: InspectorConnection,
@@ -132,9 +155,11 @@ async function logConsoleMessage(
       case 'undefined':
       case 'symbol':
       case 'bigint':
+        // Simple types are just pushed as-is
         args.push(ro.value);
         break;
       case 'function':
+        // Functions are displayed as "[Function: <name>]"
         args.push(`[Function: ${ro.description ?? '<no-description>'}]`);
         break;
       case 'object':