Skip to content

Commit 2472165

Browse files
bajrangCoderclaude
andcommitted
fix: address review feedback on build and worker startup
- pack-zip: resolve the root with fileURLToPath (import.meta.dirname needs Node 20.11+), run in-process and write via temp file + rename - esbuild: await packing in onEnd so failures exit non-zero and rebuilds never pack concurrently; start from a clean dist so stale files are not shipped - main: construct the worker inside try so an unsupported module worker shows an error and allows retry instead of leaving init stuck - worker: only dispatch own action handlers (CodeQL dynamic method call) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 8d2bb30 commit 2472165

5 files changed

Lines changed: 94 additions & 73 deletions

File tree

‎esbuild.config.mjs‎

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
import { execFile } from "node:child_process";
2-
import { cp, mkdir } from "node:fs/promises";
1+
import { cp, mkdir, rm } from "node:fs/promises";
32
import { createRequire } from "node:module";
43
import os from "node:os";
54
import path from "node:path";
65
import * as esbuild from "esbuild";
6+
import packZip from "./pack-zip.js";
77

88
const require = createRequire(import.meta.url);
99
const isServe = process.argv.includes("--serve");
@@ -37,7 +37,9 @@ function serveUrls(hosts, port) {
3737
return [...names].map((host) => `http://${host}:${port}`);
3838
}
3939

40-
async function copyPyodide() {
40+
// Starts from an empty dist so files from earlier builds are never packaged
41+
async function prepareDist() {
42+
await rm("dist", { recursive: true, force: true });
4143
const libDir = path.resolve("dist/lib");
4244
await mkdir(libDir, { recursive: true });
4345
await Promise.all(
@@ -47,16 +49,6 @@ async function copyPyodide() {
4749
);
4850
}
4951

50-
function packZip() {
51-
execFile(process.execPath, ["./pack-zip.js"], (err, stdout) => {
52-
if (err) {
53-
console.error("Error packing zip:", err);
54-
return;
55-
}
56-
console.log(stdout.trim());
57-
});
58-
}
59-
6052
// Bundles imported CSS (nesting lowered, fonts inlined) and exposes it as a string
6153
const cssTextPlugin = {
6254
name: "css-text",
@@ -85,9 +77,16 @@ const cssTextPlugin = {
8577
const zipPlugin = {
8678
name: "zip-plugin",
8779
setup(build) {
88-
build.onStart(copyPyodide);
89-
build.onEnd((result) => {
90-
if (!result.errors.length) packZip();
80+
// esbuild waits for this before finishing the build or starting a
81+
// rebuild, so packing never overlaps
82+
build.onEnd(async (result) => {
83+
if (result.errors.length) return;
84+
try {
85+
await packZip();
86+
} catch (error) {
87+
console.error("Error packing zip:", error);
88+
if (!isServe) process.exitCode = 1;
89+
}
9190
});
9291
},
9392
};
@@ -114,6 +113,8 @@ const buildConfig = {
114113
};
115114

116115
(async () => {
116+
await prepareDist();
117+
117118
if (isServe) {
118119
console.log("Starting development server...");
119120

@@ -129,6 +130,6 @@ const buildConfig = {
129130
} else {
130131
console.log("Building for production...");
131132
await esbuild.build(buildConfig);
132-
console.log("Production build complete.");
133+
if (!process.exitCode) console.log("Production build complete.");
133134
}
134135
})();

‎pack-zip.js‎

Lines changed: 70 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1,84 +1,102 @@
1-
import fs from "node:fs";
1+
import fs from "node:fs/promises";
22
import path from "node:path";
3+
import { fileURLToPath } from "node:url";
34
import jszip from "jszip";
45

5-
const root = import.meta.dirname;
6-
const iconFile = path.join(root, "icon.png");
7-
const licenseFile = path.join(root, "LICENSE");
8-
const pluginJSON = path.join(root, "plugin.json");
9-
const distFolder = path.join(root, "dist");
10-
const json = JSON.parse(fs.readFileSync(pluginJSON, "utf8"));
11-
const readmeDotMd = resolveMetadataFile(json.readme, [
12-
"readme.md",
13-
"README.md",
14-
]);
15-
const changelogDotMd = resolveMetadataFile(json.changelogs, [
16-
"changelog.md",
17-
"changelogs.md",
18-
"CHANGELOG.md",
19-
]);
20-
21-
const zip = new jszip();
22-
23-
zip.file("icon.png", fs.readFileSync(iconFile));
24-
zip.file("plugin.json", fs.readFileSync(pluginJSON));
25-
26-
if (fs.existsSync(licenseFile)) {
27-
zip.file("LICENSE", fs.readFileSync(licenseFile));
28-
}
6+
const root = path.dirname(fileURLToPath(import.meta.url));
297

30-
if (readmeDotMd) {
31-
zip.file(
32-
json.readme || path.basename(readmeDotMd),
33-
fs.readFileSync(readmeDotMd),
34-
);
35-
}
8+
/**
9+
* Packs plugin.json, metadata files and the dist folder into plugin.zip.
10+
* The archive is written to a temp file and renamed, so a dev server never
11+
* serves a partially written zip.
12+
*/
13+
export default async function packZip() {
14+
const pluginJSON = path.join(root, "plugin.json");
15+
const json = JSON.parse(await fs.readFile(pluginJSON, "utf8"));
16+
const readmeDotMd = await resolveMetadataFile(json.readme, [
17+
"readme.md",
18+
"README.md",
19+
]);
20+
const changelogDotMd = await resolveMetadataFile(json.changelogs, [
21+
"changelog.md",
22+
"changelogs.md",
23+
"CHANGELOG.md",
24+
]);
3625

37-
if (changelogDotMd) {
38-
zip.file(
39-
json.changelogs || path.basename(changelogDotMd),
40-
fs.readFileSync(changelogDotMd),
41-
);
42-
}
26+
const zip = new jszip();
4327

44-
loadFile("", distFolder);
28+
zip.file("icon.png", await fs.readFile(path.join(root, "icon.png")));
29+
zip.file("plugin.json", await fs.readFile(pluginJSON));
4530

46-
zip
47-
.generateNodeStream({ type: "nodebuffer", streamFiles: true })
48-
.pipe(fs.createWriteStream(path.join(root, "plugin.zip")))
49-
.on("finish", () => {
50-
console.log("Plugin plugin.zip written.");
51-
});
31+
const licenseFile = path.join(root, "LICENSE");
32+
if (await exists(licenseFile)) {
33+
zip.file("LICENSE", await fs.readFile(licenseFile));
34+
}
35+
36+
if (readmeDotMd) {
37+
zip.file(
38+
json.readme || path.basename(readmeDotMd),
39+
await fs.readFile(readmeDotMd),
40+
);
41+
}
5242

53-
function loadFile(base, folder) {
54-
for (const file of fs.readdirSync(folder)) {
43+
if (changelogDotMd) {
44+
zip.file(
45+
json.changelogs || path.basename(changelogDotMd),
46+
await fs.readFile(changelogDotMd),
47+
);
48+
}
49+
50+
await loadFile(zip, "", path.join(root, "dist"));
51+
52+
const output = path.join(root, "plugin.zip");
53+
const tempOutput = `${output}.tmp`;
54+
await fs.writeFile(
55+
tempOutput,
56+
await zip.generateAsync({ type: "nodebuffer", streamFiles: true }),
57+
);
58+
await fs.rename(tempOutput, output);
59+
console.log("Plugin plugin.zip written.");
60+
}
61+
62+
async function loadFile(zip, base, folder) {
63+
for (const file of await fs.readdir(folder)) {
5564
if (file === ".DS_Store" || /LICENSE.txt/.test(file)) continue;
5665

5766
const filePath = path.join(folder, file);
5867
const zipPath = path.posix.join(base, file);
5968

60-
if (fs.statSync(filePath).isDirectory()) {
69+
if ((await fs.stat(filePath)).isDirectory()) {
6170
zip.folder(zipPath);
62-
loadFile(zipPath, filePath);
71+
await loadFile(zip, zipPath, filePath);
6372
continue;
6473
}
6574

66-
zip.file(zipPath, fs.readFileSync(filePath));
75+
zip.file(zipPath, await fs.readFile(filePath));
6776
}
6877
}
6978

70-
function resolveMetadataFile(configuredPath, fallbacks) {
79+
async function resolveMetadataFile(configuredPath, fallbacks) {
7180
if (configuredPath) {
7281
const file = path.join(root, configuredPath);
73-
if (!fs.existsSync(file)) {
82+
if (!(await exists(file))) {
7483
throw new Error(`Missing plugin metadata file: ${configuredPath}`);
7584
}
7685
return file;
7786
}
7887

7988
for (const fallback of fallbacks) {
8089
const file = path.join(root, fallback);
81-
if (fs.existsSync(file)) return file;
90+
if (await exists(file)) return file;
8291
}
8392
return null;
8493
}
94+
95+
async function exists(file) {
96+
try {
97+
await fs.access(file);
98+
return true;
99+
} catch {
100+
return false;
101+
}
102+
}

‎plugin.zip‎

42 Bytes
Binary file not shown.

‎src/main.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,11 @@ class Python {
9393
this.#state = this.INITIALIZING;
9494
this.$page.settitle(strings["loading..."]);
9595
this.#worker?.terminate();
96-
this.#worker = new Worker(`${this.baseUrl}worker.js`, { type: "module" });
97-
this.#worker.onmessage = this.#workerOnMessage.bind(this);
9896

9997
try {
98+
// inside try: construction throws if module workers are unsupported
99+
this.#worker = new Worker(`${this.baseUrl}worker.js`, { type: "module" });
100+
this.#worker.onmessage = this.#workerOnMessage.bind(this);
100101
await new Promise((resolve, reject) => {
101102
this.#onInitSuccess = resolve;
102103
this.#onInitError = reject;
@@ -116,7 +117,7 @@ class Python {
116117
} catch (error) {
117118
// allow the next run to retry
118119
this.#state = this.NOT_INTIALIZED;
119-
this.print(error, "error");
120+
this.print(error?.message ?? error, "error");
120121
return false;
121122
} finally {
122123
this.$page.settitle("Python");

‎src/worker.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,8 @@ __builtins__.input = input
121121
self.onmessage = async (e) => {
122122
const { action } = e.data;
123123

124-
if (actions[action]) {
124+
// only the handlers defined above, never inherited Object methods
125+
if (Object.hasOwn(actions, action)) {
125126
await actions[action](e.data);
126127
}
127128
};

0 commit comments

Comments
 (0)