Skip to content

code style #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 7, 2023
Merged

code style #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
00298f5
code style
mbostock Nov 4, 2023
88a68e1
ignore non-executable loaders
mbostock Nov 4, 2023
73575e5
maybeStat returns undefined
mbostock Nov 7, 2023
5cecc05
findLoader
mbostock Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions src/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ 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 {getStats, prepareOutput, visitFiles, visitMarkdownFiles} from "./files.js";
import {findLoader, runLoader} from "./dataloader.js";
import {maybeStat, prepareOutput, visitFiles, visitMarkdownFiles} from "./files.js";
import {readPages} from "./navigation.js";
import {renderServerless} from "./render.js";
import {makeCLIResolver} from "./resolver.js";
import {findLoader, runCommand} from "./dataloader.js";

const EXTRA_FILES = new Map([["node_modules/@observablehq/runtime/dist/runtime.js", "_observablehq/runtime.js"]]);

Expand Down Expand Up @@ -53,15 +53,16 @@ async function build(context: CommandContext) {
for (const file of files) {
const sourcePath = join(sourceRoot, file);
const outputPath = join(outputRoot, "_file", file);
const stats = await getStats(sourcePath);
const stats = await maybeStat(sourcePath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm used to seeing "maybe" on conditional updates, not on get/find method, and when it is used it seems like it should modify a verb-object, like "maybeCommit" or "maybeCommitNotebook". We're free to create whatever convention we want, but "maybeStat" and "maybeLoader" sound strange to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think “findLoader” is a fine name, and I’m happy to revert that back. The main thing is that I wanted it to return null or undefined instead of an empty object (because this provides a more obvious way of checking existence). And I think “find” reasonably implies that it can return null or undefined (like array.find).

For “maybe” stat, it’s for parity with the underlying stat method, which at least in my experience is often used as a verb, as in “stat’ing a file”. We’ve used the “maybe” pattern extensively in Plot, but mostly for parsing/normalize user input to some trusted representation. I like the “maybe” pattern here because it indicates conditionality: you might not get the stats returned.

if (!stats) {
const {path} = await findLoader("", sourcePath);
if (!path) {
const loader = await findLoader(sourcePath);
if (!loader) {
console.error("missing referenced file", sourcePath);
continue;
}
const {path} = loader;
console.log("generate", path, "→", outputPath);
await runCommand(path, outputPath);
await runLoader(path, outputPath);
continue;
}
console.log("copy", sourcePath, "→", outputPath);
Expand Down
83 changes: 40 additions & 43 deletions src/dataloader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,54 @@
import {open} from "node:fs/promises";
import {spawn} from "node:child_process";
import {join} from "node:path";
import {getStats, prepareOutput} from "./files.js";
import {renameSync, unlinkSync} from "node:fs";
import {type Stats} from "node:fs";
import {constants, open, rename, unlink} from "node:fs/promises";
import {maybeStat, prepareOutput} from "./files.js";

const runningCommands = new Map<string, Promise<void>>();

export async function runCommand(commandPath: string, outputPath: string) {
export interface Loader {
path: string;
stats: Stats;
}

export async function runLoader(commandPath: string, outputPath: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could really be any kind of command I suppose, not just a loader.

If we want the naming to be consistent, we could change "commandPath" to loaderPath

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to make further changes here, I’d consider having findLoader return a Loader implementation with a run method. Then the caller would say loader.run instead of runLoader, and you wouldn’t have to pass in the command path again.

if (runningCommands.has(commandPath)) return runningCommands.get(commandPath);
const command = new Promise<void>((resolve, reject) => {
const command = (async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these changes fix the bug that Fil saw with multiple FileAttachments calls for the same file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does! ref. #89 (comment) ; I've built the repro and can confirm that it crashes on main and doesn't crash with this branch.

const outputTempPath = outputPath + ".tmp";
prepareOutput(outputTempPath).then(() =>
open(outputTempPath, "w").then((cacheFd) => {
const cacheFileStream = cacheFd.createWriteStream({highWaterMark: 1024 * 1024});
try {
const subprocess = spawn(commandPath, [], {
argv0: commandPath,
//cwd: dirname(commandPath), // TODO: Need to change commandPath to be relative this?
windowsHide: true,
stdio: ["ignore", "pipe", "inherit"]
// timeout: // time in ms
// signal: // abort signal
});
subprocess.stdout.on("data", (data) => cacheFileStream.write(data));
subprocess.on("error", (error) => console.error(`${commandPath}: ${error.message}`));
subprocess.on("close", (code) => {
cacheFd.close().then(() => {
if (code === 0) {
renameSync(outputTempPath, outputPath);
} else {
unlinkSync(outputTempPath);
}
resolve();
}, reject);
});
} catch (error) {
reject(error);
} finally {
runningCommands.delete(commandPath);
}
})
);
});
await prepareOutput(outputTempPath);
const cacheFd = await open(outputTempPath, "w");
const cacheFileStream = cacheFd.createWriteStream({highWaterMark: 1024 * 1024});
const subprocess = spawn(commandPath, [], {
argv0: commandPath,
//cwd: dirname(commandPath), // TODO: Need to change commandPath to be relative this?
windowsHide: true,
stdio: ["ignore", "pipe", "inherit"]
// timeout: // time in ms
// signal: // abort signal
});
subprocess.stdout.pipe(cacheFileStream);
const code = await new Promise((resolve, reject) => {
subprocess.on("error", reject); // (error) => console.error(`${commandPath}: ${error.message}`));
subprocess.on("close", resolve);
});
await cacheFd.close();
if (code === 0) {
await rename(outputTempPath, outputPath);
} else {
await unlink(outputTempPath);
}
})();
command.finally(() => runningCommands.delete(commandPath));
runningCommands.set(commandPath, command);
return command;
}

export async function findLoader(root: string, name: string) {
export async function findLoader(name: string): Promise<Loader | undefined> {
// TODO: It may be more efficient use fs.readdir
for (const ext of [".js", ".ts", ".sh"]) {
const path = join(root, name) + ext;
const stats = await getStats(path);
if (stats) return {path, stats};
const path = name + ext;
const stats = await maybeStat(path);
if (stats && stats.mode & constants.S_IXUSR) {
return {path, stats};
}
}
return {};
}
7 changes: 3 additions & 4 deletions src/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type {Stats} from "node:fs";
import {accessSync, constants, statSync} from "node:fs";
import {accessSync, constants, statSync, type Stats} from "node:fs";
import {mkdir, readdir, stat} from "node:fs/promises";
import {dirname, extname, join, normalize, relative} from "node:path";
import {isNodeError} from "./error.js";
Expand Down Expand Up @@ -52,13 +51,13 @@ export async function* visitFiles(root: string): AsyncGenerator<string> {
}
}

export async function getStats(path: string): Promise<Stats | undefined> {
// Like fs.stat, but returns undefined instead of throwing ENOENT if not found.
export async function maybeStat(path: string): Promise<Stats | undefined> {
try {
return await stat(path);
} catch (error) {
if (!isNodeError(error) || error.code !== "ENOENT") throw error;
}
return;
}

export async function prepareOutput(outputPath: string): Promise<void> {
Expand Down
51 changes: 20 additions & 31 deletions src/preview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
import type {WatchListener} from "node:fs";
import {watch, type FSWatcher} from "node:fs";
import {watch, type FSWatcher, type WatchListener} from "node:fs";
import {access, constants, readFile, stat} from "node:fs/promises";
import {createServer, type IncomingMessage, type RequestListener} from "node:http";
import {basename, dirname, extname, join, normalize} from "node:path";
import {fileURLToPath} from "node:url";
import {parseArgs} from "node:util";
import send from "send";
import {WebSocketServer, type WebSocket} from "ws";
import {findLoader, runLoader} from "./dataloader.js";
import {HttpError, isHttpError, isNodeError} from "./error.js";
import type {ParseResult} from "./markdown.js";
import {diffMarkdown, readMarkdown} from "./markdown.js";
import {maybeStat} from "./files.js";
import {diffMarkdown, readMarkdown, type ParseResult} from "./markdown.js";
import {readPages} from "./navigation.js";
import {renderPreview} from "./render.js";
import type {CellResolver} from "./resolver.js";
import {makeCLIResolver} from "./resolver.js";
import {findLoader, runCommand} from "./dataloader.js";
import {getStats} from "./files.js";
import {makeCLIResolver, type CellResolver} from "./resolver.js";

const publicRoot = join(dirname(fileURLToPath(import.meta.url)), "..", "public");
const cacheRoot = join(dirname(fileURLToPath(import.meta.url)), "..", ".observablehq", "cache");
Expand Down Expand Up @@ -70,18 +67,15 @@ class Server {
}

// Look for a data loader for this file.
const {path: loaderPath, stats: loaderStat} = await findLoader(this.root, path);
if (loaderStat) {
const loader = await findLoader(filepath);
if (loader) {
const cachePath = join(this.cacheRoot, filepath);
const cacheStat = await getStats(cachePath);
if (cacheStat && cacheStat.mtimeMs > loaderStat.mtimeMs) {
const cacheStat = await maybeStat(cachePath);
if (cacheStat && cacheStat.mtimeMs > loader.stats.mtimeMs) {
send(req, filepath, {root: this.cacheRoot}).pipe(res);
return;
}
if (!(loaderStat.mode & constants.S_IXUSR)) {
throw new HttpError("Data loader is not executable", 404);
}
await runCommand(loaderPath, cachePath);
await runLoader(loader.path, cachePath);
send(req, filepath, {root: this.cacheRoot}).pipe(res);
return;
}
Expand Down Expand Up @@ -167,10 +161,10 @@ class FileWatchers {
const fileset = [...new Set(this.files.map(({name}) => name))];
for (const name of fileset) {
const watchPath = await FileWatchers.getWatchPath(this.root, name);
let prevState = await getStats(watchPath);
let prevState = await maybeStat(watchPath);
this.watchers.push(
watch(watchPath, async () => {
const newState = await getStats(watchPath);
const newState = await maybeStat(watchPath);
// Ignore if the file was truncated or not modified.
if (prevState?.mtimeMs === newState?.mtimeMs || newState?.size === 0) return;
prevState = newState;
Expand All @@ -182,10 +176,10 @@ class FileWatchers {

static async getWatchPath(root: string, name: string) {
const path = join(root, name);
const stats = await getStats(path);
const stats = await maybeStat(path);
if (stats?.isFile()) return path;
const {path: loaderPath, stats: loaderStat} = await findLoader(root, name);
return loaderStat?.isFile() ? loaderPath : path;
const loader = await findLoader(path);
return loader?.stats.isFile() ? loader.path : path;
}

close() {
Expand All @@ -195,16 +189,11 @@ class FileWatchers {
}

function resolveDiffs(diff: ReturnType<typeof diffMarkdown>, resolver: CellResolver): ReturnType<typeof diffMarkdown> {
for (const item of diff) {
if (item.type === "add") {
for (const addItem of item.items) {
if (addItem.type === "cell" && "databases" in addItem) {
Object.assign(addItem, resolver(addItem));
}
}
}
}
return diff;
return diff.map((item) =>
item.type === "add"
? {...item, items: item.items.map((addItem) => (addItem.type === "cell" ? resolver(addItem) : addItem))}
: item
);
}

function handleWatch(socket: WebSocket, options: {root: string; resolver: CellResolver}) {
Expand Down
8 changes: 5 additions & 3 deletions src/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import {computeHash} from "./hash.js";
import {type FileReference, type ImportReference} from "./javascript.js";
import {resolveImport} from "./javascript/imports.js";
import type {CellPiece} from "./markdown.js";
import {parseMarkdown, type ParseResult} from "./markdown.js";
import {parseMarkdown, type CellPiece, type ParseResult} from "./markdown.js";

export interface Render {
html: string;
Expand Down Expand Up @@ -60,7 +59,10 @@ ${
}<link rel="stylesheet" type="text/css" href="https://fonts.googleapis.com/css2?family=Source+Serif+Pro:ital,wght@0,400;0,600;0,700;1,400;1,600;1,700&display=swap">
<link rel="stylesheet" type="text/css" href="/_observablehq/style.css">
${Array.from(getImportPreloads(parseResult))
.concat(parseResult.imports.filter(({name}) => name.startsWith("./")).map(({name}) => `/_file/${name.slice(2)}`))
.concat(
parseResult.imports.filter(({name}) => name.startsWith("./")).map(({name}) => `/_file/${name.slice(2)}`),
parseResult.cells.some((cell) => cell.databases?.length) ? "/_observablehq/database.js" : []
)
.map((href) => `<link rel="modulepreload" href="${href}">`)
.join("\n")}
<script type="module">
Expand Down
13 changes: 6 additions & 7 deletions src/resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {homedir} from "os";
import {join} from "path";
import {createHmac} from "node:crypto";
import {readFile} from "node:fs/promises";
import type {CellPiece} from "./markdown.js";
import {homedir} from "node:os";
import {join} from "node:path";
import {type CellPiece} from "./markdown.js";

export type CellResolver = (cell: CellPiece) => CellPiece;

Expand Down Expand Up @@ -41,7 +41,7 @@ export async function readDatabaseProxyConfig(): Promise<DatabaseProxyConfig | n
let observableConfig;
try {
observableConfig = JSON.parse(await readFile(configFile, "utf-8")) as ObservableConfig | null;
} catch (error) {
} catch {
// Ignore missing config file
}
return observableConfig && observableConfig[key];
Expand All @@ -65,13 +65,13 @@ function decodeSecret(secret: string): Record<string, string> {
function encodeToken(payload: {name: string}, secret): string {
const data = JSON.stringify(payload);
const hmac = createHmac("sha256", Buffer.from(secret, "hex")).update(data).digest();
return `${Buffer.from(data).toString("base64") + "." + Buffer.from(hmac).toString("base64")}`;
return `${Buffer.from(data).toString("base64")}.${Buffer.from(hmac).toString("base64")}`;
}

export async function makeCLIResolver(): Promise<CellResolver> {
const config = await readDatabaseProxyConfig();
return (cell: CellPiece): CellPiece => {
if ("databases" in cell && cell.databases !== undefined) {
if (cell.databases !== undefined) {
cell = {
...cell,
databases: cell.databases.map((ref) => {
Expand All @@ -81,7 +81,6 @@ export async function makeCLIResolver(): Promise<CellResolver> {
url.protocol = db.ssl !== "disabled" ? "https:" : "http:";
url.host = db.host;
url.port = String(db.port);
url.toString();
return {
...ref,
token: encodeToken(ref, db.secret),
Expand Down