fix file path resolution (#120)

* Add build command test

* Add a second build test

* fix file path resolution

* don’t treat imports as files

* normalize top-level import path

* findFiles; fix snapshot generation

* fix tests

* fold execute into build

* fix import preloads, again

* restore leading slash

* fix early exit

* remove obsolete comment

* quiet build during test

* fix crash without sidebar

* include build changed in failure artifacts

---------

Co-authored-by: Wiltse Carpenter <[email protected]>

Author: mbostock

.github/workflows/test.yml

@@ -24,4 +24,6 @@ jobs:
         if: failure()
         with:
           name: test-output-changes
-          path: test/output/*-changed.*
+          path: |
+            test/output/*-changed.*
+            test/output/build/*-changed/

.gitignore

@@ -2,5 +2,7 @@
 dist/
 docs/.observablehq/cache
 node_modules/
+test/input/build/*/.observablehq/cache
 test/output/*-changed.*
+test/output/build/*-changed/
 yarn-error.log

bin/observable.ts

@@ -5,14 +5,14 @@ const command = process.argv.splice(2, 1)[0];
 switch (command) {
   case "-v":
   case "--version": {
-    import("../package.json").then(({version}: any) => console.log(version));
+    await import("../package.json").then(({version}: any) => console.log(version));
     break;
   }
   case "build":
-    import("../src/build.js");
+    await import("../src/build.js").then((build) => build.build());
     break;
   case "preview":
-    import("../src/preview.js");
+    await import("../src/preview.js");
     break;
   default:
     console.error(`Usage: observable <command>`);

package.json

@@ -30,8 +30,8 @@
   },
   "dependencies": {
     "@observablehq/runtime": "^5.9.4",
-    "acorn": "^8.10.0",
-    "acorn-walk": "^8.2.0",
+    "acorn": "^8.11.2",
+    "acorn-walk": "^8.3.0",
     "fast-array-diff": "^1.1.0",
     "fast-deep-equal": "^3.1.3",
     "gray-matter": "^4.0.3",
@@ -53,6 +53,7 @@
     "@types/ws": "^8.5.6",
     "@typescript-eslint/eslint-plugin": "^6.7.3",
     "@typescript-eslint/parser": "^6.7.3",
+    "d3-array": "^3.2.4",
     "eslint": "^8.50.0",
     "eslint-config-prettier": "^9.0.0",
     "eslint-plugin-prettier": "^5.0.0",

public/client.js

@@ -191,7 +191,7 @@ export function define(cell) {
   v.define(outputs.length ? `cell ${id}` : null, inputs, body);
   variables.push(v);
   for (const o of outputs) variables.push(main.define(o, [`cell ${id}`], (exports) => exports[o]));
-  for (const f of files) attachedFiles.set(f.name, {url: String(new URL(`/_file/${f.name}`, location)), mimeType: f.mimeType}); // prettier-ignore
+  for (const f of files) attachedFiles.set(f.name, {url: `/_file${(new URL(f.name, location)).pathname}`, mimeType: f.mimeType}); // prettier-ignore
   for (const d of databases) databaseTokens.set(d.name, d);
 }
 
@@ -300,8 +300,8 @@ export function open({hash} = {}) {
   }
 }
 
-{
-  const toggle = document.querySelector("#observablehq-sidebar-toggle");
+const toggle = document.querySelector("#observablehq-sidebar-toggle");
+if (toggle) {
   let indeterminate = toggle.indeterminate;
   toggle.onclick = () => {
     const matches = matchMedia("(min-width: calc(640px + 4rem + 0.5rem + 240px + 2rem))").matches;

src/build.ts

@@ -1,18 +1,26 @@
+import {existsSync} from "node:fs";
 import {access, constants, copyFile, readFile, writeFile} from "node:fs/promises";
 import {basename, dirname, join, normalize, relative} from "node:path";
 import {cwd} from "node:process";
 import {fileURLToPath} from "node:url";
 import {parseArgs} from "node:util";
 import {Loader} from "./dataloader.js";
-import {maybeStat, prepareOutput, visitFiles, visitMarkdownFiles} from "./files.js";
+import {prepareOutput, visitFiles, visitMarkdownFiles} from "./files.js";
 import {readPages} from "./navigation.js";
 import {renderServerless} from "./render.js";
 import {makeCLIResolver} from "./resolver.js";
 
 const EXTRA_FILES = new Map([["node_modules/@observablehq/runtime/dist/runtime.js", "_observablehq/runtime.js"]]);
 
-async function build(context: CommandContext) {
-  const {sourceRoot, outputRoot} = context;
+export interface CommandContext {
+  sourceRoot: string;
+  outputRoot: string;
+  verbose?: boolean;
+  addPublic?: boolean;
+}
+
+export async function build(context: CommandContext = makeCommandContext()) {
+  const {sourceRoot, outputRoot, verbose = true, addPublic = true} = context;
 
   // Make sure all files are readable before starting to write output files.
   for await (const sourceFile of visitMarkdownFiles(sourceRoot)) {
@@ -25,67 +33,66 @@ async function build(context: CommandContext) {
   const resolver = await makeCLIResolver();
   for await (const sourceFile of visitMarkdownFiles(sourceRoot)) {
     const sourcePath = join(sourceRoot, sourceFile);
-    const outputPath = join(outputRoot, join(dirname(sourceFile), basename(sourceFile, ".md") + ".html"));
-    console.log("render", sourcePath, "→", outputPath);
+    const outputPath = join(outputRoot, dirname(sourceFile), basename(sourceFile, ".md") + ".html");
+    if (verbose) console.log("render", sourcePath, "→", outputPath);
     const path = `/${join(dirname(sourceFile), basename(sourceFile, ".md"))}`;
     const render = renderServerless(await readFile(sourcePath, "utf-8"), {
       root: sourceRoot,
       path,
       pages,
       resolver
     });
-    files.push(...render.files.map((f) => f.name));
+    files.push(...render.files.map((f) => join(dirname(sourceFile), f.name)));
+    files.push(...render.imports.map((f) => join(dirname(sourceFile), f.name)));
     await prepareOutput(outputPath);
     await writeFile(outputPath, render.html);
   }
 
   // Copy over the public directory.
-  const publicRoot = join(dirname(relative(cwd(), fileURLToPath(import.meta.url))), "..", "public");
-  for await (const publicFile of visitFiles(publicRoot)) {
-    const sourcePath = join(publicRoot, publicFile);
-    const outputPath = join(outputRoot, "_observablehq", publicFile);
-    console.log("copy", sourcePath, "→", outputPath);
-    await prepareOutput(outputPath);
-    await copyFile(sourcePath, outputPath);
+  if (addPublic) {
+    const publicRoot = join(dirname(relative(cwd(), fileURLToPath(import.meta.url))), "..", "public");
+    for await (const publicFile of visitFiles(publicRoot)) {
+      const sourcePath = join(publicRoot, publicFile);
+      const outputPath = join(outputRoot, "_observablehq", publicFile);
+      if (verbose) console.log("copy", sourcePath, "→", outputPath);
+      await prepareOutput(outputPath);
+      await copyFile(sourcePath, outputPath);
+    }
   }
 
   // Copy over the referenced files.
   for (const file of files) {
     let sourcePath = join(sourceRoot, file);
     const outputPath = join(outputRoot, "_file", file);
-    const stats = await maybeStat(sourcePath);
-    if (!stats) {
+    if (!existsSync(sourcePath)) {
       const loader = Loader.find(sourceRoot, file);
       if (!loader) {
         console.error("missing referenced file", sourcePath);
         continue;
       }
-      process.stdout.write(`generate ${loader.path} → `);
+      if (verbose) process.stdout.write(`generate ${loader.path} → `);
       sourcePath = join(sourceRoot, await loader.load());
-      console.log(sourcePath);
+      if (verbose) console.log(sourcePath);
     }
-    console.log("copy", sourcePath, "→", outputPath);
+    if (verbose) console.log("copy", sourcePath, "→", outputPath);
     await prepareOutput(outputPath);
     await copyFile(sourcePath, outputPath);
   }
 
   // Copy over required distribution files from node_modules.
   // TODO: Note that this requires that the build command be run relative to the node_modules directory.
-  for (const [sourcePath, targetFile] of EXTRA_FILES) {
-    const outputPath = join(outputRoot, targetFile);
-    console.log("copy", sourcePath, "→", outputPath);
-    await prepareOutput(outputPath);
-    await copyFile(sourcePath, outputPath);
+  if (addPublic) {
+    for (const [sourcePath, targetFile] of EXTRA_FILES) {
+      const outputPath = join(outputRoot, targetFile);
+      if (verbose) console.log("copy", sourcePath, "→", outputPath);
+      await prepareOutput(outputPath);
+      await copyFile(sourcePath, outputPath);
+    }
   }
 }
 
 const USAGE = `Usage: observable build [--root dir] [--output dir]`;
 
-interface CommandContext {
-  sourceRoot: string;
-  outputRoot: string;
-}
-
 function makeCommandContext(): CommandContext {
   const {values} = parseArgs({
     options: {
@@ -110,9 +117,3 @@ function makeCommandContext(): CommandContext {
     outputRoot: normalize(values.output).replace(/\/$/, "")
   };
 }
-
-await (async function () {
-  const context = makeCommandContext();
-  await build(context);
-  process.exit(0);
-})();

src/dataloader.ts

@@ -80,7 +80,7 @@ export class Loader {
       const loaderStat = await maybeStat(this.path);
       const cacheStat = await maybeStat(cachePath);
       if (cacheStat && cacheStat.mtimeMs > loaderStat!.mtimeMs) return outputPath;
-      const tempPath = join(".observablehq", "cache", `${this.targetPath}.${process.pid}`);
+      const tempPath = join(this.sourceRoot, ".observablehq", "cache", `${this.targetPath}.${process.pid}`);
       await prepareOutput(tempPath);
       const tempFd = await open(tempPath, "w");
       const tempFileStream = tempFd.createWriteStream({highWaterMark: 1024 * 1024});

src/javascript.ts

@@ -23,6 +23,15 @@ export interface ImportReference {
   type: "global" | "local";
 }
 
+export interface Feature {
+  type: "FileAttachment" | "DatabaseClient" | "Secret";
+  name: string;
+}
+
+export interface Identifier {
+  name: string;
+}
+
 export interface Transpile {
   id: string;
   inputs?: string[];
@@ -47,10 +56,12 @@ export function transpileJavaScript(input: string, options: ParseOptions): Trans
   const {id, root, sourcePath} = options;
   try {
     const node = parseJavaScript(input, options);
-    const databases = node.features.filter((f) => f.type === "DatabaseClient").map((f) => ({name: f.name}));
+    const databases = node.features
+      .filter((f) => f.type === "DatabaseClient")
+      .map((f): DatabaseReference => ({name: f.name}));
     const files = node.features
       .filter((f) => f.type === "FileAttachment")
-      .map((f) => ({name: f.name, mimeType: mime.getType(f.name)}));
+      .map((f): FileReference => ({name: f.name, mimeType: mime.getType(f.name)}));
     const inputs = Array.from(new Set<string>(node.references.map((r) => r.name)));
     const output = new Sourcemap(input);
     trim(output, input);
@@ -103,12 +114,12 @@ export const parseOptions: Options = {ecmaVersion: 13, sourceType: "module"};
 
 export interface JavaScriptNode {
   body: Node;
-  declarations: {name: string}[] | null;
-  references: {name: string}[];
-  features: {type: unknown; name: string}[];
-  imports: {type: "global" | "local"; name: string}[];
-  expression: boolean;
-  async: boolean;
+  declarations: Identifier[] | null; // null for expressions that can’t declare top-level variables, a.k.a outputs
+  references: Identifier[]; // the unbound references, a.k.a. inputs
+  features: Feature[];
+  imports: ImportReference[];
+  expression: boolean; // is this an expression or a program cell?
+  async: boolean; // does this use top-level await?
 }
 
 function parseJavaScript(input: string, options: ParseOptions): JavaScriptNode {
@@ -121,8 +132,8 @@ function parseJavaScript(input: string, options: ParseOptions): JavaScriptNode {
   const body = expression ?? (Parser.parse(input, parseOptions) as any);
   const references = findReferences(body, globals, input);
   const declarations = expression ? null : findDeclarations(body, globals, input);
-  const {imports, features: importFeatures} = findImports(body, root, sourcePath);
-  const features = [...importFeatures, ...findFeatures(body, root, sourcePath, references, input)];
+  const imports = findImports(body, root, sourcePath);
+  const features = findFeatures(body, root, sourcePath, references, input);
   return {
     body,
     declarations,
@@ -137,7 +148,7 @@ function parseJavaScript(input: string, options: ParseOptions): JavaScriptNode {
 // Parses a single expression; like parseExpressionAt, but returns null if
 // additional input follows the expression.
 function maybeParseExpression(input, options) {
-  const parser = new Parser(options, input, 0);
+  const parser = new (Parser as any)(options, input, 0); // private constructor
   parser.nextToken();
   try {
     const node = (parser as any).parseExpression();

src/javascript/features.js renamed to src/javascript/features.ts

@@ -1,10 +1,10 @@
 import {simple} from "acorn-walk";
+import {type Feature} from "../javascript.js";
+import {isLocalImport} from "./imports.js";
 import {syntaxError} from "./syntaxError.js";
-import {isLocalImport} from "./imports.ts";
-import {dirname, join} from "node:path";
 
 export function findFeatures(node, root, sourcePath, references, input) {
-  const features = [];
+  const features: Feature[] = [];
 
   simple(node, {
     CallExpression(node) {
@@ -13,9 +13,10 @@ export function findFeatures(node, root, sourcePath, references, input) {
         arguments: args,
         arguments: [arg]
       } = node;
+
       // Promote fetches with static literals to file attachment references.
       if (isLocalFetch(node, references, root, sourcePath)) {
-        features.push({type: "FileAttachment", name: join(dirname(sourcePath), getStringLiteralValue(arg))});
+        features.push({type: "FileAttachment", name: getStringLiteralValue(arg)});
         return;
       }
 
@@ -34,6 +35,7 @@ export function findFeatures(node, root, sourcePath, references, input) {
       if (args.length !== 1 || !isStringLiteral(arg)) {
         throw syntaxError(`${callee.name} requires a single literal string argument`, node, input);
       }
+
       features.push({type: callee.name, name: getStringLiteralValue(arg)});
     }
   });

src/javascript/fetches.js renamed to src/javascript/fetches.ts

@@ -1,14 +1,12 @@
-import {Parser} from "acorn";
-import type {Node} from "acorn";
+import {Parser, type Node} from "acorn";
 import {simple} from "acorn-walk";
 import {readFileSync} from "node:fs";
-import {dirname, join} from "node:path";
-import {type JavaScriptNode, parseOptions} from "../javascript.js";
+import {dirname, join, normalize} from "node:path";
+import {parseOptions, type ImportReference, type JavaScriptNode} from "../javascript.js";
 import {getStringLiteralValue, isStringLiteral} from "./features.js";
 
 export function findImports(body: Node, root: string, sourcePath: string) {
-  const imports: {name: string; type: "global" | "local"}[] = [];
-  const features: {name: string; type: string}[] = [];
+  const imports: ImportReference[] = [];
   const paths = new Set<string>();
 
   simple(body, {
@@ -22,23 +20,22 @@ export function findImports(body: Node, root: string, sourcePath: string) {
     if (isStringLiteral(node.source)) {
       const value = getStringLiteralValue(node.source);
       if (isLocalImport(value, root, sourcePath)) {
-        findLocalImports(join(dirname(sourcePath), value));
+        findLocalImports(normalize(value));
       } else {
         imports.push({name: value, type: "global"});
       }
     }
   }
 
   // If this is an import of a local ES module, recursively parse the module to
-  // find transitive imports.
-  // path is the full URI path without /_file
+  // find transitive imports. The path is always relative to the source path of
+  // the Markdown file, even across transitive imports.
   function findLocalImports(path) {
     if (paths.has(path)) return;
     paths.add(path);
     imports.push({type: "local", name: path});
-    features.push({type: "FileAttachment", name: path});
     try {
-      const input = readFileSync(join(root, path), "utf-8");
+      const input = readFileSync(join(root, dirname(sourcePath), path), "utf-8");
       const program = Parser.parse(input, parseOptions);
       simple(program, {
         ImportDeclaration: findLocalImport,
@@ -52,9 +49,8 @@ export function findImports(body: Node, root: string, sourcePath: string) {
     function findLocalImport(node) {
       if (isStringLiteral(node.source)) {
         const value = getStringLiteralValue(node.source);
-        if (isLocalImport(value, root, path)) {
-          const subpath = join(dirname(path), value);
-          findLocalImports(subpath);
+        if (isLocalImport(value, root, sourcePath)) {
+          findLocalImports(join(dirname(path), value));
         } else {
           imports.push({name: value, type: "global"});
           // non-local imports don't need to be promoted to file attachments
@@ -63,7 +59,7 @@ export function findImports(body: Node, root: string, sourcePath: string) {
     }
   }
 
-  return {imports, features};
+  return imports;
 }
 
 // TODO parallelize multiple static imports