Skip to content

fix: improve runtime placeholder handling #4447

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
GordonSmith merged 1 commit into from
Sep 5, 2025
Merged

fix: improve runtime placeholder handling #4447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 2 additions & 3 deletions packages/observablehq-compiler/index-kit.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@
<title>Home</title>
<meta charset="utf-8">
<title>ObservableHQ Kit Preview</title>
<!-- Google Fonts: Roboto -->
<!-- <style>
<style>
body,
html {
font-family: "Segoe UI", Verdana, Arial, Helvetica, sans-serif;
}
</style> -->
</style>
</head>

<body>
Expand Down
4 changes: 2 additions & 2 deletions packages/observablehq-compiler/src/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

import { type Notebook, type Definition, compile as compileKit, fixRelativeUrl, isRelativePath, obfuscatedImport } from "./kit/index.ts";
import { type Notebook, type Definition, compileNotebook, fixRelativeUrl, isRelativePath, obfuscatedImport } from "./kit/index.ts";
import { ohq, splitModule } from "./observable-shim.ts";
import { parseCell, ParsedImportCell } from "./cst.ts";
import { Writer } from "./writer.ts";
Expand Down Expand Up @@ -330,7 +330,7 @@ export async function compile(notebookOrOjs: ohq.Notebook, options?: CompileOpti
export async function compile(notebookOrOjs: string, options?: CompileOptions): Promise<NotebookFunc>;
export async function compile(notebookOrOjs: Notebook | ohq.Notebook | string, { baseUrl = ".", importMode = "precompiled" }: CompileOptions = {}) {
if (isNotebookKit(notebookOrOjs)) {
return compileKit(notebookOrOjs);
return compileNotebook(notebookOrOjs);
} else if (typeof notebookOrOjs === "string") {
notebookOrOjs = ojs2notebook(notebookOrOjs);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/observablehq-compiler/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ export type { ohq } from "./observable-shim.ts";

export * from "./compiler.ts";
export { ojs2notebook, omd2notebook, omd2notebookKit, ojs2notebookKit, download } from "./util.ts";
export { compile as compileKit, html2notebook, notebook2html } from "./kit/index.ts";
export * from "./kit/index.ts";
export * from "./writer.ts";
65 changes: 40 additions & 25 deletions packages/observablehq-compiler/src/kit/compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,56 @@

import { type Notebook, transpile } from "@observablehq/notebook-kit";
import { type Notebook, type Cell, transpile } from "@observablehq/notebook-kit";
import { type Definition } from "@observablehq/notebook-kit/runtime";
import { constructFunction } from "./util.ts";

export interface CompileKitOptions {
inline?: boolean;
}

export function compile(notebook: Notebook, options: CompileKitOptions = { inline: true }): Definition[] {
export function compileCell(cell: Cell, options: CompileKitOptions = { inline: true }): Definition[] {
const retVal: Definition[] = [];
let id = 1;
for (const cell of notebook.cells) {
try {
const compiled = transpile(cell);
retVal.push({
id: id++,
...compiled,
body: options.inline ? constructFunction(compiled.body, `cell_${id}`) : compiled.body,
});
if (cell.pinned) {
const compiled = transpile({
...cell,
mode: "md",
value: `\
const sourceIDOffset = 1000000;
Copy link

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

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

The magic number 1000000 should be defined as a named constant to improve code readability and maintainability. Consider defining it as const SOURCE_ID_OFFSET = 1000000; at the top of the file.

Copilot uses AI. Check for mistakes.
try {
const compiled = transpile(cell);
retVal.push({
id: cell.id,
...compiled,
body: options.inline ? constructFunction(compiled.body, `cell_${cell.id}`) : compiled.body,
});
if (cell.pinned) {
const compiled = transpile({
...cell,
mode: "md",
value: `\
\`\`\`${cell.mode}
${cell.value}
\`\`\``
});
retVal.push({
id: id++,
...compiled,
body: options.inline ? constructFunction(compiled.body, `cell_${id}`) : compiled.body,
});
}
} catch (error) {
console.error(`Error compiling cell ${id}:`, error);
});
retVal.push({
id: sourceIDOffset + cell.id,
...compiled,
body: options.inline ? constructFunction(compiled.body, `cell_source_${sourceIDOffset + cell.id}`) : compiled.body,
});
}
} catch (error) {
console.error(`Error compiling cell ${cell.id}:`, error);
}
return retVal;
}

export function compileNotebook(notebook: Notebook, options: CompileKitOptions = { inline: true }): Definition[] {
const retVal: Definition[] = [];
for (const cell of notebook.cells) {
const cellDefs = compileCell(cell, options);
retVal.push(...cellDefs);
}
return retVal;
}

export function resetCellIDs(notebook: Notebook, start: number = 0, increment: number = 1): Notebook {
for (const cell of notebook.cells) {
cell.id = start;
start += increment;
}
return notebook;
}
1 change: 0 additions & 1 deletion packages/observablehq-compiler/src/kit/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export * from "./util.ts";
export * from "./compiler.ts";
export * from "./util.ts";
66 changes: 48 additions & 18 deletions packages/observablehq-compiler/src/kit/runtime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { type Definition } from "./index.ts";
import "@observablehq/notebook-kit/index.css";
import "@observablehq/notebook-kit/theme-air.css";

export { DefineState };

export class NotebookRuntime extends NotebookRuntimeBase {

stateById = new Map<number, DefineState>();
Expand All @@ -16,23 +18,50 @@ export class NotebookRuntime extends NotebookRuntimeBase {
return this.stateById.has(cellId);
}

async add(cellId: number, definition: Definition, placeholderDiv: HTMLDivElement): Promise<void> {
let state: DefineState | undefined = this.stateById.get(cellId);
if (state) {
this.remove(cellId);
async add(definition: Definition): Promise<HTMLDivElement> {
if (this.stateById.has(definition.id)) {
throw new Error(`Cell with id ${definition.id} already exists`);
}
state = { root: placeholderDiv, expanded: [], variables: [] };
this.stateById.set(cellId, state);
const placeholderDiv = document.createElement("div");
placeholderDiv.className = "observablehq observablehq--cell";
const state = { root: placeholderDiv, expanded: [], variables: [] };
this.stateById.set(definition.id, state);
this.define(state, definition);
await this.runtime._computeNow();
return state.root;
}

async remove(cellId: number): Promise<void> {
async update(definition: Definition): Promise<HTMLDivElement> {
const state = this.stateById.get(definition.id);
if (!state) {
throw new Error(`Cell with id ${definition.id} does not exist`);
}
await this.clear(definition.id);
this.define(state, definition);
await this.runtime._computeNow();
return state.root;
}

async clear(cellId: number): Promise<void> {
const state = this.stateById.get(cellId);
if (!state) return;
if (!state) {
throw new Error(`Cell with id ${cellId} does not exist`);
}
[...state.variables].forEach(v => v.delete());
await this.runtime._computeNow();
state.variables = [];
state.expanded = [];
state.root.innerHTML = "";
}

async remove(cellId: number): Promise<void> {
const state = this.stateById.get(cellId);
if (!state) {
throw new Error(`Cell with id ${cellId} does not exist`);
}
void this.clear(cellId);
Copy link

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

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

The void operator is being used to explicitly ignore the Promise returned by this.clear(cellId). However, this could lead to issues if the clear operation fails. Consider awaiting the operation or handling potential errors.

Suggested change
void this.clear(cellId);
await this.clear(cellId);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@GordonSmith GordonSmith Sep 5, 2025

Choose a reason for hiding this comment

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

The void is documenting that await is not needed here.

this.stateById.delete(cellId);
state.root?.remove();
state.root.remove();
await this.runtime._computeNow();
}

Expand All @@ -44,14 +73,15 @@ export class NotebookRuntime extends NotebookRuntimeBase {
await this.runtime._computeNow();
}

render(definitions: Definition[], target: HTMLElement) {
definitions.forEach(definition => {
const placeholderDiv = document.createElement("div");
placeholderDiv.id = `cell-${definition.id}`;
placeholderDiv.className = "observablehq observablehq--cell";
this.stateById.set(definition.id, { root: placeholderDiv, expanded: [], variables: [] });
this.define(this.stateById.get(definition.id)!, definition);
target.appendChild(placeholderDiv);
});
async render(definitions: Definition[], target: HTMLElement) {
for (const definition of definitions) {
let observableDiv: HTMLDivElement;
if (this.stateById.has(definition.id)) {
observableDiv = await this.update(definition);
} else {
observableDiv = await this.add(definition);
}
target.appendChild(observableDiv);
}
}
}
22 changes: 11 additions & 11 deletions packages/observablehq-compiler/tests/index-notebookkit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { compileKit, html2notebook } from "@hpcc-js/observablehq-compiler";
import { compileNotebook, html2notebook } from "@hpcc-js/observablehq-compiler";
import { NotebookRuntime } from "@hpcc-js/observablehq-compiler/runtime";

// import "@observablehq/notebook-kit/theme-air.css";
Expand All @@ -19,13 +19,13 @@ import { NotebookRuntime } from "@hpcc-js/observablehq-compiler/runtime";

export async function testHtml(target) {

const element = document.getElementById(target);
if (!element) {
throw new Error(`Element with id ${target} not found`);
}
const element = document.getElementById(target);
if (!element) {
throw new Error(`Element with id ${target} not found`);
}

// const html = await fetch("../tests/albers-usa-projection.txt");
const html = `\
// const html = await fetch("../tests/albers-usa-projection.txt");
const html = `\
<!doctype html>
<notebook theme="slate">
<title>Observable Notebooks 2.0 System guide</title>
Expand Down Expand Up @@ -210,8 +210,8 @@ export async function testHtml(target) {
</script>
</notebook>
`;
const notebook = html2notebook(html);
const definitions = compileKit(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
const notebook = html2notebook(html);
const definitions = compileNotebook(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
}
68 changes: 34 additions & 34 deletions packages/observablehq-compiler/tests/index-notebookkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { omd2notebookKit, ojs2notebookKit, compileKit, html2notebook } from "../src/index.ts";
import { omd2notebookKit, ojs2notebookKit, compileNotebook, html2notebook } from "../src/index.ts";
import { NotebookRuntime } from "../src/kit/runtime.ts";

// import "@observablehq/notebook-kit/theme-air.css";
// import "@observablehq/notebook-kit/theme-coffee.css";
// import "@observablehq/notebook-kit/theme-cotton.css";
// import "@observablehq/notebook-kit/theme-deep-space.css";
// import "@observablehq/notebook-kit/theme-glacier.css";
// import "@observablehq/notebook-kit/theme-ink.css";
import "@observablehq/notebook-kit/theme-ink.css";
// import "@observablehq/notebook-kit/theme-midnight.css";
// import "@observablehq/notebook-kit/theme-near-midnight.css";
// import "@observablehq/notebook-kit/theme-ocean-floor.css";
Expand All @@ -19,28 +19,28 @@ import { NotebookRuntime } from "../src/kit/runtime.ts";

export async function testHtml(target: string): Promise<void> {

const element = document.getElementById(target) as HTMLDivElement;
if (!element) {
throw new Error(`Element with id ${target} not found`);
}

// const html = await fetch("../tests/albers-usa-projection.txt");
const html = await fetch("../tests/system-guide.txt")
.then((response) => {
return response.text();
});
const notebook = html2notebook(html);
const definitions = compileKit(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
const element = document.getElementById(target) as HTMLDivElement;
if (!element) {
throw new Error(`Element with id ${target} not found`);
}

// const html = await fetch("../tests/albers-usa-projection.txt");
const html = await fetch("../tests/system-guide.txt")
.then((response) => {
return response.text();
});
const notebook = html2notebook(html);
const definitions = compileNotebook(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
}

export function testOmd(target: string): void {
const element = document.getElementById(target) as HTMLDivElement;
if (!element) {
throw new Error(`Element with id ${target} not found`);
}
const omd2 = `\
const element = document.getElementById(target) as HTMLDivElement;
if (!element) {
throw new Error(`Element with id ${target} not found`);
}
const omd2 = `\

## Imports (dot)

Expand Down Expand Up @@ -331,18 +331,18 @@ value

`;

const notebook = omd2notebookKit(omd2);
const definitions = compileKit(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
const notebook = omd2notebookKit(omd2);
const definitions = compileKit(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
}

export function testOjs(target: string): void {
const element = document.getElementById(target) as HTMLDivElement;
if (!element) {
throw new Error(`Element with id ${target} not found`);
}
const ojs = `\
const element = document.getElementById(target) as HTMLDivElement;
if (!element) {
throw new Error(`Element with id ${target} not found`);
}
const ojs = `\
2 * 3 * 7
{
let sum = 0;
Expand All @@ -357,8 +357,8 @@ md \`# lib.ojs\`;
mo = 38 + 40;
`;

const notebook = ojs2notebookKit(ojs);
const definitions = compileKit(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
const notebook = ojs2notebookKit(ojs);
const definitions = compileKit(notebook);
const runtime = new NotebookRuntime();
runtime.render(definitions, element);
}