fix: Windows support (#89)

* ci: Add windows test build step
* fix: Windows support
* Don't replace : on unix

---------

Co-authored-by: Rafał Rzepecki <divided.mind@gmail.com>

.github/workflows/ci.yml

@@ -41,6 +41,16 @@ jobs:
         env:
           POSTGRES_URL: postgres://postgres:postgres@localhost:5432
 
+  windows-test:
+    runs-on: windows-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-node@v3
+      - run: yarn
+      - run: yarn prepack
+      - run: yarn test
+      - run: node test/smoketest.mjs
+
   release:
     runs-on: ubuntu-latest
     permissions:

src/PackageMatcher.ts

@@ -1,25 +1,37 @@
 import { resolve } from "node:path";
 import { fileURLToPath } from "node:url";
+import fwdSlashPath from "./util/fwdSlashPath";
 
 export default class PackageMatcher extends Array<Package> {
   constructor(
     private root: string,
     packages: Package[],
   ) {
+    // Normalize path separators
+    packages.forEach((p) => {
+      p.path = fwdSlashPath(p.path);
+      if (p.exclude)
+        for (let i = 0; i < p.exclude.length; i++) p.exclude[i] = fwdSlashPath(p.exclude[i]);
+    });
+
     super(...packages);
-    this.resolved = new Map(packages.map(({ path }) => [path, resolve(root, path)]));
+    this.resolved = new Map(packages.map(({ path }) => [path, fwdSlashPath(resolve(root, path))]));
   }
 
   private resolved: Map<string, string>;
 
   private resolve(path: string) {
-    return this.resolved.get(path) ?? resolve(this.root, path);
+    return this.resolved.get(path) ?? fwdSlashPath(resolve(this.root, path));
   }
 
   match(path: string): Package | undefined {
     if (path.startsWith("file:")) path = fileURLToPath(path);
-    const pkg = this.find((pkg) => path.startsWith(this.resolve(pkg.path)));
-    return pkg?.exclude?.find((ex) => path.includes(ex)) ? undefined : pkg;
+
+    // Make sure passed path is forward slashed
+    const fixedPath = fwdSlashPath(path);
+
+    const pkg = this.find((pkg) => fixedPath.startsWith(this.resolve(pkg.path)));
+    return pkg?.exclude?.find((ex) => fixedPath.includes(ex)) ? undefined : pkg;
   }
 }
 

src/Recording.ts

@@ -217,7 +217,8 @@ function makeAppMapFilename(name: string): string {
   return name + ".appmap.json";
 }
 
+const charsToQuote = process.platform == "win32" ? /[/\\:<>*"|?]/g : /[/\\]/g;
 function quotePathSegment(value: string): string {
   // note replacing spaces isn't strictly necessary improves UX
-  return value.replaceAll(/[/\\]/g, "-").replaceAll(" ", "_");
+  return value.replaceAll(charsToQuote, "-").replaceAll(" ", "_");
 }

src/__tests__/config.test.ts

@@ -8,6 +8,7 @@ import YAML from "yaml";
 
 import PackageMatcher from "../PackageMatcher";
 import { Config } from "../config";
+import { fixAbsPath } from "../hooks/__tests__/fixAbsPath";
 
 tmp.setGracefulCleanup();
 
@@ -98,10 +99,12 @@ describe(Config, () => {
 describe(PackageMatcher, () => {
   it("matches packages", () => {
     const pkg = { path: ".", exclude: ["node_modules", ".yarn"] };
-    const matcher = new PackageMatcher("/test/app", [pkg]);
-    expect(matcher.match("/test/app/lib/foo.js")).toEqual(pkg);
-    expect(matcher.match("/other/app/lib/foo.js")).toBeUndefined();
-    expect(matcher.match("/test/app/node_modules/lib/foo.js")).toBeUndefined();
-    expect(matcher.match("/test/app/.yarn/lib/foo.js")).toBeUndefined();
+    const matcher = new PackageMatcher(fixAbsPath("/test/app"), [pkg]);
+    expect(matcher.match(fixAbsPath("/test/app/lib/foo.js"))).toEqual(pkg);
+    if (process.platform == "win32")
+      expect(matcher.match(fixAbsPath("\\test\\app\\lib\\foo.js"))).toEqual(pkg);
+    expect(matcher.match(fixAbsPath("/other/app/lib/foo.js"))).toBeUndefined();
+    expect(matcher.match(fixAbsPath("/test/app/node_modules/lib/foo.js"))).toBeUndefined();
+    expect(matcher.match(fixAbsPath("/test/app/.yarn/lib/foo.js"))).toBeUndefined();
   });
 });

src/bin.ts

@@ -3,6 +3,7 @@ import { ChildProcess, spawn } from "node:child_process";
 import { accessSync, readFileSync, writeFileSync } from "node:fs";
 import { dirname, join, resolve } from "node:path";
 import { kill, pid } from "node:process";
+import { pathToFileURL } from "node:url";
 
 import stripJsonComments from "strip-json-comments";
 import YAML from "yaml";
@@ -14,7 +15,10 @@ import forwardSignals from "./util/forwardSignals";
 import { readPkgUp } from "./util/readPkgUp";
 
 const registerPath = resolve(__dirname, "../dist/register.js");
-const loaderPath = resolve(__dirname, "../dist/loader.js");
+// We need to convert c: to file:// in Windows because:
+// "Error: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader.
+// On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'"
+const loaderPath = pathToFileURL(resolve(__dirname, "../dist/loader.js")).href;
 
 export function main() {
   const [cmd, ...args] = process.argv.slice(2);
@@ -46,7 +50,14 @@ export function main() {
     }
   } else {
     // it's a command, spawn it
-    child = spawn(cmd, args, { stdio: "inherit" });
+
+    // We need to give shell: true in Windows because we get "Error: spawn yarn ENOENT"
+    // for example when the cmd is yarn. Looks like it needs the full path of the
+    // executable otherwise.
+    // Related articles:
+    // - https://stackoverflow.com/questions/37459717/error-spawn-enoent-on-windows
+    // - https://github.com/nodejs/node/issues/7367#issuecomment-238594729
+    child = spawn(cmd, args, { stdio: "inherit", shell: process.platform == "win32" });
   }
 
   forwardSignals(child);

src/classMap.ts

@@ -1,5 +1,4 @@
 import assert from "node:assert";
-import { sep } from "node:path";
 
 import type AppMap from "./AppMap";
 import type { FunctionInfo, SourceLocation } from "./registry";
@@ -13,7 +12,8 @@ export function makeClassMap(funs: Iterable<FunctionInfo>): AppMap.ClassMap {
   // sorting isn't strictly necessary, but it provides for a stable output
   for (const fun of sortFunctions(funs)) {
     if (!fun.location) continue;
-    const pkgs = fun.location.path.replace(/\..+$/, "").split(sep).reverse();
+    // fun.location can contain "/" as separator even in Windows
+    const pkgs = fun.location.path.replace(/\..+$/, "").split(/[/\\]/).reverse();
     if (pkgs.length > 1) pkgs.shift(); // remove the file name (e.g. "foo.js")
 
     let [tree, classes]: FNode = [root, {}];

src/config.ts

@@ -32,7 +32,7 @@ export class Config {
     const config = readConfigFile(this.configPath);
     this.default = !config;
 
-    this.relativeAppmapDir = config?.appmap_dir ?? join("tmp", "appmap");
+    this.relativeAppmapDir = config?.appmap_dir ?? "tmp/appmap";
 
     this.appName = config?.name ?? targetPackage()?.name ?? basename(root);
 

src/hooks/__tests__/fixAbsPath.ts

@@ -0,0 +1,6 @@
+// Add drive parts for absolute paths in Windows.
+export function fixAbsPath(pathOrFileUrl: string) {
+  if (process.platform != "win32") return pathOrFileUrl;
+  if (pathOrFileUrl.startsWith("/") || pathOrFileUrl.startsWith("\\")) return "F:" + pathOrFileUrl;
+  return pathOrFileUrl.replace("file:///", "file:///F:/");
+}

src/hooks/__tests__/instrument.test.ts

@@ -2,23 +2,25 @@ import { full as walk } from "acorn-walk";
 import { ESTree, parse } from "meriyah";
 
 import config from "../../config";
+import { fixAbsPath } from "./fixAbsPath";
+import * as instrument from "../instrument";
 import PackageMatcher from "../../PackageMatcher";
 import * as registry from "../../registry";
-import * as instrument from "../instrument";
 
 describe(instrument.shouldInstrument, () => {
-  jest.replaceProperty(config, "root", "/test");
+  jest.replaceProperty(config, "root", fixAbsPath("/test"));
   jest.replaceProperty(
     config,
     "packages",
-    new PackageMatcher("/test", [{ path: ".", exclude: ["node_modules"] }]),
+    new PackageMatcher(fixAbsPath("/test"), [{ path: ".", exclude: ["node_modules"] }]),
   );
+
   test.each([
     ["node:test", false],
-    ["file:///test/test.json", false],
-    ["file:///var/test.js", false],
-    ["file:///test/test.js", true],
-    ["file:///test/node_modules/test.js", false],
+    [fixAbsPath("file:///test/test.json"), false],
+    [fixAbsPath("file:///var/test.js"), false],
+    [fixAbsPath("file:///test/test.js"), true],
+    [fixAbsPath("file:///test/node_modules/test.js"), false],
   ])("%s", (url, expected) => expect(instrument.shouldInstrument(new URL(url))).toBe(expected));
 });
 

src/hooks/__tests__/jest.test.ts

@@ -1,4 +1,6 @@
 import { parse } from "meriyah";
+
+import { fixAbsPath } from "./fixAbsPath";
 import * as jestHook from "../jest";
 import transform from "../../transform";
 
@@ -53,9 +55,11 @@ describe(jestHook.patchRuntime, () => {
 describe(jestHook.transformJest, () => {
   it("pushes jest transformed code through appmap hooks", () => {
     jest.mocked(transform).mockReturnValue("transformed test code");
-    const result = jestHook.transformJest.call(undefined, () => "test code", ["/test/test.js"]);
+    const result = jestHook.transformJest.call(undefined, () => "test code", [
+      fixAbsPath("/test/test.js"),
+    ]);
     expect(result).toBe("transformed test code");
-    expect(transform).toBeCalledWith("test code", new URL("file:///test/test.js"));
+    expect(transform).toBeCalledWith("test code", new URL(fixAbsPath("file:///test/test.js")));
   });
 });
 

src/hooks/vitest.ts

@@ -114,7 +114,7 @@ function patchRunTest(fd: ESTree.FunctionDeclaration) {
       // Statement: return await import(".../vitest.js").wrapRunTest(function runTest(...) {...}, arguments);
       ret(
         call_(
-          member(awaitImport(__filename), identifier(wrapRunTest.name)),
+          member(awaitImport(pathToFileURL(__filename).href), identifier(wrapRunTest.name)),
           { ...fd, type: "FunctionExpression" },
           args_,
         ),
@@ -137,7 +137,7 @@ function patchRunModule(md: ESTree.MethodDefinition) {
   const transformCodeStatement = assignment(
     identifier("transformed"),
     call_(
-      member(awaitImport(__filename), identifier(transformCode.name)),
+      member(awaitImport(pathToFileURL(__filename).href), identifier(transformCode.name)),
       identifier("transformed"),
       memberId("context", "__filename"),
     ),

src/util/__tests__/commonPathPrefix.test.ts

@@ -8,4 +8,7 @@ describe(commonPathPrefix, () => {
     [["/foo/bar", "/foo/barbara"], "/foo/"],
     [["/foo/bar", "/other/dir"], "/"],
   ])("%j => %s", (paths, expected) => expect(commonPathPrefix(paths)).toBe(expected));
+
+  if (process.platform == "win32")
+    expect(commonPathPrefix(["c:\\foo\\bar", "C:\\Foo\\Baz"])).toBe("c:\\foo\\");
 });

src/util/commonPathPrefix.ts

@@ -4,10 +4,17 @@ export default function commonPathPrefix(paths: string[]): string {
   const [first, ...others] = paths;
   if (!first) return "";
 
+  const isEqual = (a: string, b: string) => {
+    return process.platform == "win32"
+      ? a.localeCompare(b, "en", { sensitivity: "base" }) == 0
+      : a == b;
+  };
+
   let prefixLen = 0;
   for (let i = 0; i < first.length; i++)
-    if (!others.every((path) => path[i] === first[i])) break;
-    else if (first[i] === sep) prefixLen = i;
+    if (!others.every((path) => isEqual(path[i], first[i]))) break;
+    // We occasionally convert back slash to forward slash even in Windows
+    else if (first[i] === sep || first[i] === "/") prefixLen = i;
 
   return first.slice(0, prefixLen + 1);
 }

src/util/fwdSlashPath.ts

@@ -0,0 +1,4 @@
+export default function fwdSlashPath(path: string): string {
+  if (process.platform != "win32") return path;
+  return path?.replaceAll("\\", "/");
+}

test/__snapshots__/next.test.ts.snap

@@ -112,7 +112,7 @@ exports[`mapping a Next.js appmap 1`] = `
     },
     "version": "1.12",
   },
-  "./tmp/appmap/requests/<timestamp 1> -about.appmap.json": {
+  "./tmp/appmap/requests/<timestamp 1>_-about.appmap.json": {
     "classMap": [
       {
         "children": [
@@ -129,7 +129,7 @@ exports[`mapping a Next.js appmap 1`] = `
             "type": "class",
           },
         ],
-        "name": "about",
+        "name": "pages",
         "type": "package",
       },
     ],

test/helpers.ts

@@ -8,6 +8,7 @@ import caller from "caller";
 import { globSync } from "fast-glob";
 
 import type AppMap from "../src/AppMap";
+import fwdSlashPath from "../src/util/fwdSlashPath";
 
 const binPath = resolve(__dirname, "../bin/appmap-node.js");
 
@@ -29,13 +30,13 @@ export function spawnAppmapNode(...args: string[]): ChildProcessWithoutNullStrea
   return result;
 }
 
-let target = cwd();
+let target = fwdSlashPath(cwd());
 
 export function testDir(path: string) {
-  target = resolve(path);
+  target = fwdSlashPath(resolve(path));
 }
 
-beforeEach(() => rmSync(resolveTarget("tmp"), { recursive: true, force: true }));
+beforeEach(() => rmSync(resolveTarget("tmp"), { recursive: true, force: true, maxRetries: 3 }));
 
 export function resolveTarget(...path: string[]): string {
   return resolve(target, ...path);
@@ -55,7 +56,7 @@ type AppMap = object & Record<"events", unknown>;
 
 export function readAppmap(path?: string): AppMap.AppMap {
   if (!path) {
-    const files = globSync(resolve(target, "tmp/**/*.appmap.json"));
+    const files = globSync(fwdSlashPath(resolve(target, "tmp/**/*.appmap.json")));
     expect(files.length).toBe(1);
     [path] = files;
   }
@@ -80,7 +81,7 @@ export function fixAppmap(map: unknown): AppMap.AppMap {
 }
 
 export function readAppmaps(): Record<string, AppMap.AppMap> {
-  const files = globSync(resolve(target, "tmp/**/*.appmap.json"));
+  const files = globSync(fwdSlashPath(resolve(target, "tmp/**/*.appmap.json")));
   const maps = files.map<[string, AppMap.AppMap]>((path) => [fixPath(path), readAppmap(path)]);
   return Object.fromEntries(maps);
 }
@@ -128,13 +129,13 @@ beforeEach(() => (timestampId = 0));
 
 function fixTimeStamps(str: string): string {
   return str.replaceAll(
-    /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z/g,
-    (ts) => (timestamps[ts] ||= `<timestamp ${timestampId++}>`),
+    /\d{4}-\d{2}-\d{2}T\d{2}[:-]\d{2}[:-]\d{2}\.\d{3}Z/g,
+    (ts) => (timestamps[ts.replaceAll(":", "-")] ||= `<timestamp ${timestampId++}>`),
   );
 }
 
 function fixPath(path: string): string {
-  return fixTimeStamps(path.replace(target, "."));
+  return fixTimeStamps(fwdSlashPath(path).replace(target, "."));
 }
 
 function fixClassMap(classMap: unknown[]) {

test/httpServer.test.ts

@@ -7,6 +7,8 @@ integrationTest("mapping Express.js requests", async () => {
   expect.assertions(1);
   const server = await spawnServer("express.js");
   await makeRequests();
+  // Wait for the last request to finish
+  await awaitStdoutOnData(server, "api-bar.appmap.json");
   await killServer(server);
   expect(readAppmaps()).toMatchSnapshot();
 });
@@ -15,6 +17,8 @@ integrationTest("mapping node:http requests", async () => {
   expect.assertions(1);
   const server = await spawnServer("vanilla.js");
   await makeRequests();
+  // Wait for the last request to finish
+  await awaitStdoutOnData(server, "api-bar.appmap.json");
   await killServer(server);
   expect(readAppmaps()).toMatchSnapshot();
 });
@@ -39,6 +43,14 @@ integrationTest("mapping node:http requests with remote recording", async () =>
   await killServer(server);
 });
 
+async function awaitStdoutOnData(server: ChildProcessWithoutNullStreams, searchString: string) {
+  await new Promise<void>((r) =>
+    server.stdout.on("data", (chunk: Buffer) => {
+      if (chunk.toString().includes(searchString)) r();
+    }),
+  );
+}
+
 async function makeRequests() {
   await makeRequest("");
   await makeRequest("/nonexistent");
@@ -58,11 +70,7 @@ async function makeRequests() {
 
 async function spawnServer(script: string) {
   const server = spawnAppmapNode(script);
-  await new Promise<void>((r) =>
-    server.stdout.on("data", (chunk: Buffer) => {
-      if (chunk.toString().includes("listening")) r();
-    }),
-  );
+  await awaitStdoutOnData(server, "listening");
   return server;
 }