Merge pull request #875 from pmcelhaney/reload-dependent-modules

reload dependent modules

.changeset/fluffy-doors-sell.md

@@ -0,0 +1,5 @@
+---
+"counterfact": patch
+---
+
+a lot of refactoring of the code that loads modules

.eslintrc.cjs

@@ -4,6 +4,15 @@ const rules = {
   "@microsoft/sdl/no-html-method": "off",
   "@typescript-eslint/lines-around-comment": "off",
   "@typescript-eslint/naming-convention": "off",
+
+  "id-length": [
+    "error",
+    {
+      exceptions: ["$"],
+      min: 2,
+    },
+  ],
+
   "import/default": "off",
 
   "import/namespace": "off",
@@ -31,6 +40,7 @@ const rules = {
   ],
 
   "node/file-extension-in-import": "off",
+
   "node/no-callback-literal": "off",
 
   "node/no-missing-import": "off",

.mise.toml

@@ -0,0 +1,2 @@
+[tools]
+node = "20"

package.json

@@ -99,6 +99,7 @@
     "node-fetch": "3.3.2",
     "open": "10.1.0",
     "patch-package": "8.0.0",
+    "precinct": "^12.1.1",
     "prettier": "3.2.5",
     "typescript": "5.4.5"
   }

src/server/context-registry.ts

@@ -15,6 +15,8 @@ export class ContextRegistry {
 
   private readonly cache = new Map<string, Context>();
 
+  private readonly seen = new Set<string>();
+
   public constructor() {
     this.add("/", {});
   }
@@ -28,11 +30,18 @@ export class ContextRegistry {
     return this.entries.get(path) ?? this.find(parentPath(path));
   }
 
+  // eslint-disable-next-line max-statements
   public update(path: string, updatedContext?: Context): void {
     if (updatedContext === undefined) {
       return;
     }
 
+    if (!this.seen.has(path)) {
+      this.seen.add(path);
+      this.add(path, updatedContext);
+      return;
+    }
+
     const context = this.find(path);
 
     for (const property in updatedContext) {

src/server/module-dependency-graph.ts

@@ -0,0 +1,59 @@
+import { dirname, resolve } from "node:path";
+
+import precinct from "precinct";
+
+export class ModuleDependencyGraph {
+  private readonly dependents = new Map<string, Set<string>>();
+
+  private loadDependencies(path: string) {
+    try {
+      return precinct.paperwork(path);
+    } catch {
+      return [];
+    }
+  }
+
+  private clearDependents(path: string) {
+    this.dependents.forEach((group) => {
+      group.delete(path);
+    });
+  }
+
+  public load(path: string) {
+    this.clearDependents(path);
+    for (const dependency of this.loadDependencies(path)) {
+      if (!dependency.startsWith(".")) {
+        return;
+      }
+
+      const key = resolve(dirname(path), dependency);
+      if (!this.dependents.has(key)) {
+        this.dependents.set(key, new Set());
+      }
+      this.dependents.get(key)?.add(path);
+    }
+  }
+
+  // eslint-disable-next-line max-statements, sonarjs/cognitive-complexity
+  public dependentsOf(path: string) {
+    const marked = new Set<string>();
+    const dependents = new Set<string>();
+    const queue = [path];
+
+    while (queue.length > 0) {
+      const file = queue.shift();
+      if (file !== undefined && !marked.has(file)) {
+        marked.add(file);
+        const fileDependents = this.dependents.get(file);
+        if (fileDependents) {
+          for (const dependent of fileDependents) {
+            dependents.add(dependent);
+            queue.push(dependent);
+          }
+        }
+      }
+    }
+
+    return dependents;
+  }
+}

src/server/module-loader.ts

@@ -1,7 +1,7 @@
 import { once } from "node:events";
-import { type Dirent, existsSync } from "node:fs";
+import { existsSync } from "node:fs";
 import fs from "node:fs/promises";
-import nodePath from "node:path";
+import nodePath, { basename, dirname } from "node:path";
 
 import { type FSWatcher, watch } from "chokidar";
 import createDebug from "debug";
@@ -26,18 +26,6 @@ function isContextModule(
   return "Context" in module && typeof module.Context === "function";
 }
 
-function reportLoadError(error: unknown, fileUrl: string) {
-  if (
-    String(error) ===
-    "SyntaxError: Identifier 'Context' has already been declared"
-  ) {
-    // Not sure why Node throws this error. It doesn't seem to matter.
-    return;
-  }
-
-  process.stdout.write(`\nError loading ${fileUrl}:\n~~${String(error)}~~\n`);
-}
-
 export class ModuleLoader extends EventTarget {
   private readonly basePath: string;
 
@@ -95,32 +83,7 @@ export class ModuleLoader extends EventTarget {
           this.dispatchEvent(new Event("remove"));
         }
 
-        debug("importing module: %s", pathName);
-        this.uncachedImport(pathName)
-          // eslint-disable-next-line promise/prefer-await-to-then
-          .then((endpoint) => {
-            this.dispatchEvent(new Event(eventName));
-
-            if (pathName.includes("_.context")) {
-              this.contextRegistry.update(
-                parts.dir,
-
-                // @ts-expect-error TS says Context has no constructable signatures but that's not true?
-                // eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/consistent-type-assertions
-                new (endpoint as ContextModule).Context(),
-              );
-              return "context";
-            }
-
-            // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
-            this.registry.add(url, endpoint as Module);
-
-            return "path";
-          })
-          // eslint-disable-next-line promise/prefer-await-to-then
-          .catch((error: unknown) => {
-            reportLoadError(error, pathName);
-          });
+        void this.loadEndpoint(pathName);
       },
     );
     await once(this.watcher, "ready");
@@ -167,41 +130,48 @@ export class ModuleLoader extends EventTarget {
       const fullPath = nodePath
         .join(this.basePath, directory, file.name)
         .replaceAll("\\", "/");
-      await this.loadEndpoint(fullPath, directory, file);
+
+      await this.loadEndpoint(fullPath);
     });
 
     await Promise.all(imports);
   }
 
-  private async loadEndpoint(
-    fullPath: string,
-    directory: string,
-    file: Dirent,
-  ) {
+  // eslint-disable-next-line max-statements
+  private async loadEndpoint(pathName: string) {
+    debug("importing module: %s", pathName);
+
+    const directory = dirname(pathName.slice(this.basePath.length)).replaceAll(
+      "\\",
+      "/",
+    );
+
+    const url = `/${nodePath.join(
+      directory,
+      nodePath.parse(basename(pathName)).name,
+    )}`
+      .replaceAll("\\", "/")
+      .replaceAll(/\/+/gu, "/");
+
     try {
       // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
-      const endpoint: ContextModule | Module = (await this.uncachedImport(
-        fullPath,
-      )) as ContextModule | Module;
+      const endpoint = (await this.uncachedImport(pathName)) as
+        | ContextModule
+        | Module;
+
+      this.dispatchEvent(new Event("add"));
 
-      if (file.name.includes("_.context")) {
+      if (basename(pathName).startsWith("_.context")) {
         if (isContextModule(endpoint)) {
-          this.contextRegistry.add(
-            `/${directory.replaceAll("\\", "/")}`,
+          this.contextRegistry.update(
+            directory,
 
             // @ts-expect-error TS says Context has no constructable signatures but that's not true?
             // eslint-disable-next-line @typescript-eslint/no-unsafe-argument
             new endpoint.Context(),
           );
         }
       } else {
-        const url = `/${nodePath.join(
-          directory,
-          nodePath.parse(file.name).name,
-        )}`
-          .replaceAll("\\", "/")
-          .replaceAll(/\/+/gu, "/");
-
         // eslint-disable-next-line @typescript-eslint/consistent-type-assertions
         this.registry.add(url, endpoint as Module);
       }
@@ -214,7 +184,7 @@ export class ModuleLoader extends EventTarget {
         return;
       }
 
-      process.stdout.write(`\nError loading ${fullPath}:\n${String(error)}\n`);
+      process.stdout.write(`\nError loading ${pathName}:\n${String(error)}\n`);
     }
   }
 }

src/server/precinct.d.ts

@@ -0,0 +1,3 @@
+declare module 'precinct' {
+  export function paperwork(path: string): string[];
+}

test/server/code-generator.test.ts

@@ -1,6 +1,3 @@
-/* eslint-disable @typescript-eslint/no-unsafe-assignment */
-/* eslint-disable @typescript-eslint/no-unsafe-argument */
-/* eslint-disable @typescript-eslint/no-unsafe-call */
 import { usingTemporaryFiles } from "using-temporary-files";
 
 import { CodeGenerator } from "../../src/server/code-generator.js";

test/server/context-registry.test.ts

@@ -10,7 +10,7 @@ describe("a context registry", () => {
 
     const registry = new ContextRegistry();
 
-    registry.add("/hello", helloContext);
+    registry.update("/hello", helloContext);
 
     expect(registry.find("/hello")).toBe(helloContext);
   });
@@ -60,7 +60,7 @@ describe("a context registry", () => {
 
     const registry = new ContextRegistry();
 
-    registry.add("/", originalContext);
+    registry.update("/", originalContext);
     originalContext.increment();
 
     expect(registry.find("/").count).toBe(1);
@@ -87,7 +87,6 @@ it("updates context properties if they changed in the code", () => {
 
     [key: string]: unknown;
   }
-
   class UpdatedContext implements Context {
     public prop1 = "original";
 
@@ -104,7 +103,7 @@ it("updates context properties if they changed in the code", () => {
 
   const registry = new ContextRegistry();
 
-  registry.add("/", new OriginalContext());
+  registry.update("/", new OriginalContext());
 
   const context = registry.find("/");
 

test/server/deterimine-module-kind.test.ts

@@ -1,5 +1,3 @@
-/* eslint-disable @typescript-eslint/no-unsafe-argument */
-/* eslint-disable @typescript-eslint/no-unsafe-call */
 // To do: add types to usingTemporaryFiles package
 
 import { usingTemporaryFiles } from "using-temporary-files";