Skip to content
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.

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

Better dev experience for JS generated file hashes #7975

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions pixi.toml
Original file line number Diff line number Diff line change
Expand Up @@ -280,10 +280,14 @@ js-setup = "npm i -g yarn"
js-install = { cmd = "yarn install --cwd rerun_js", depends_on = ["js-setup"] }

# Build JS packages
js-build-base = { cmd = "yarn --cwd rerun_js/web-viewer run build", depends_on = [
js-build-base = { cmd = "yarn --cwd rerun_js/web-viewer run build:no-check", depends_on = [
"js-install",
] }
js-build-all = { cmd = "yarn --cwd rerun_js workspaces run build", depends_on = [
js-build-all = { cmd = "yarn --cwd rerun_js workspaces run build:no-check", depends_on = [
"js-install",
] }

js-update-hashes = { cmd = "node rerun_js/web-viewer/build-wasm.mjs --update-hashes", depends_on = [
Copy link
Member

Choose a reason for hiding this comment

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

docstring! Update what hashes? Why and when would they need updating?

"js-install",
] }

Expand Down
5 changes: 3 additions & 2 deletions rerun_js/web-viewer-react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
],
"scripts": {
"build:types": "tsc --noEmit && dts-buddy",
"build": "npm run build:types"
"build": "npm run build:types",
"build:no-check": "npm run build"
Copy link
Member Author

Choose a reason for hiding this comment

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

This does the same thing as build, it's only here for compatibility with running the same command across all packages in the yarn workspace (yarn --cwd rerun_js workspaces run build:no-check would fail on the web-viewer-react package otherwise)

Copy link
Member

Choose a reason for hiding this comment

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

That's a great comment - it should be comited :)

Copy link
Member Author

@jprochazk jprochazk Nov 2, 2024

Choose a reason for hiding this comment

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

yes, it would be nice if it was possible to put comments in JSON. i'm not sure where else to put it

Copy link
Member

Choose a reason for hiding this comment

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

oh right 🤦 God damn JSON

},
"repository": {
"type": "git",
Expand Down Expand Up @@ -47,4 +48,4 @@
"dts-buddy": "^0.3.0",
"typescript": "^5.2.2"
}
}
}
76 changes: 54 additions & 22 deletions rerun_js/web-viewer/build-wasm.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ import * as util from "node:util";
const __filename = path.resolve(fileURLToPath(import.meta.url));
const __dirname = path.dirname(__filename);

const exec = (cmd) => {
const exec = (/** @type {string} */ cmd) => {
console.log(cmd);
child_process.execSync(cmd, { cwd: __dirname, stdio: "inherit" });
};

function buildWebViewer(mode) {
function buildWebViewer(/** @type {"debug" | "release"} */ mode) {
switch (mode) {
case "debug": {
return exec(
Expand All @@ -31,9 +31,9 @@ function buildWebViewer(mode) {
}
}

async function re_viewer_js(mode) {
async function re_viewer_js(/** @type {"debug" | "release"} */ mode) {
let code = fs.readFileSync(path.join(__dirname, "re_viewer.js"), "utf-8");
await checkHash(mode, "re_viewer.js", code);
const valid = await isHashValid(mode, "re_viewer.js", code);

// this transforms the module, wrapping it in a default-exported function.
// calling the function produces a new "instance" of the module, because
Expand Down Expand Up @@ -87,11 +87,12 @@ return Object.assign(__wbg_init, { initSync, deinit }, __exports);
code = code.replace(closure_dtors, closure_dtors_patch);

fs.writeFileSync(path.join(__dirname, "re_viewer.js"), code);

return valid;
}

async function re_viewer_d_ts(mode) {
function re_viewer_d_ts() {
let code = fs.readFileSync(path.join(__dirname, "re_viewer.d.ts"), "utf-8");
await checkHash(mode, "re_viewer.d.ts", code);

// this transformation just re-exports WebHandle and adds a default export inside the `.d.ts` file

Expand All @@ -111,34 +112,59 @@ async function hash(data) {
.join("");
}

async function checkHash(mode, id, data) {
async function isHashValid(
/** @type {"debug" | "release"} */ mode,
/** @type {string} */ id,
/** @type {string} */ data,
) {
const storedHash = hashes?.[mode]?.[id];
const computedHash = await hash(new TextEncoder().encode(data));

if (updateHashes) {
if (options.updateHashes) {
hashes[mode] ??= {};
hashes[mode][id] = computedHash;
return;
return true;
}

if (storedHash !== computedHash) {
return storedHash === computedHash;
}

async function run(/** @type {"debug" | "release"} */ mode) {
buildWebViewer(mode);
const valid = await re_viewer_js(mode);
re_viewer_d_ts(mode);

if (options.checkHashes && !valid) {
console.error(`
==============================================================
Output of "${id}" changed.
Update the \`build-wasm.mjs\` script to handle the new output,
then run \`pixi run node build-wasm.mjs --update-hashes\`.
Output of "re_viewer.js" has changed.

The the output of \`wasm-bindgen\` is unstable, but we depend on it
for the web viewer. In order to make sure this didn't break anything,
you should:

1) Run \`pixi run js-build-base\`, and
2) Inspect the generated \`rerun_js/web-viewer/re_viewer.js\` file.

Most of the time, the hashes change because:

- Some field offsets changed,
- You added/removed/changed a closure passed into JS,
- You added/removed/changed a new exported symbol

Ideally, you would diff \`re_viewer.js\` against the same file generated
on the \`main\` branch to make sure that this is the case. You can disable
the hash checks by running:

pixi run node rerun_js/web-viewer/build-wasm.mjs --mode release --no-check

Run \`pixi run js-update-hashes\` once you're sure that there are no issues.
==============================================================
`);
`);
process.exit(1);
}
}

async function run(mode) {
buildWebViewer(mode);
await re_viewer_js(mode);
await re_viewer_d_ts(mode);
}

const args = util.parseArgs({
options: {
mode: {
Expand All @@ -147,10 +173,16 @@ const args = util.parseArgs({
"update-hashes": {
type: "boolean",
},
"no-check": {
type: "boolean",
},
},
});

let updateHashes = !!args.values["update-hashes"];
let options = {
updateHashes: !!args.values["update-hashes"],
checkHashes: !args.values["no-check"],
};
let hashes;
try {
hashes = JSON.parse(
Expand All @@ -161,7 +193,7 @@ try {
}

try {
if (updateHashes) {
if (options.updateHashes) {
await run("release");
await run("debug");
fs.writeFileSync(
Expand Down
1 change: 1 addition & 0 deletions rerun_js/web-viewer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"build:wasm": "node build-wasm.mjs --mode release",
"build:js": "tsc && node bundle.mjs",
"build": "npm run build:wasm && npm run build:js",
"build:no-check": "npm run build:wasm -- --no-check && npm run build:js",
"build:wasm:debug": "node build-wasm.mjs --mode debug",
"build:debug": "npm run build:wasm:debug && npm run build:js"
},
Expand Down
Loading