Skip to content

feat: ✨ Add WASM web demo and fix wasm conversion panics#133

Open
Sube-py wants to merge 1 commit intodeveloper0hye:mainfrom
Sube-py:main
Open

feat: ✨ Add WASM web demo and fix wasm conversion panics#133
Sube-py wants to merge 1 commit intodeveloper0hye:mainfrom
Sube-py:main

Conversation

@Sube-py
Copy link
Copy Markdown

@Sube-py Sube-py commented Mar 9, 2026

No description provided.

Signed-off-by: zhangluguang <z397503810@gmail.com>
Copy link
Copy Markdown
Owner

@developer0hye developer0hye left a comment

Choose a reason for hiding this comment

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

Code Review — PR #133

Thanks for the contribution! The WASM web demo and panic-safety improvements are useful, but there are several issues that need to be addressed before merging. Detailed inline comments below.


CRITICAL

1. crate-type = ["cdylib", "rlib"] applied unconditionally

File: crates/office2pdf/Cargo.toml

Adding cdylib to the default crate-type affects every build target, not just WASM. On native builds, Cargo will now produce an additional shared library artifact (*.dylib / *.so / *.dll), increasing build time and disk usage for all users who never use WASM.

Cargo does not support conditional crate-type, so the options are:

  • Remove cdylib from the lib crate and create a separate crates/office2pdf-wasm crate (thin wrapper that re-exports with crate-type = ["cdylib"]) — this is the standard pattern used by projects like typst, wasm-bindgen examples, etc.
  • Or document that cdylib is only meaningful when targeting wasm32-unknown-unknown and accept the minor native overhead.

The first option is strongly preferred since it keeps the lib crate clean.

2. Indentation error in streaming path

File: crates/office2pdf/src/lib.rs (around the streaming single-chunk path)

            metrics: Some(ConvertMetrics {
                parse_duration,
            codegen_duration: Duration::ZERO,  // ← wrong indent (de-indented by 4 spaces)
            compile_duration: Duration::ZERO,  // ← wrong indent
                total_duration,

This is a formatting error — cargo fmt should have caught it. Per project rules, cargo fmt -- --check must pass before committing.


HIGH

3. Unnecessary Cargo.toml reformatting

File: crates/office2pdf/Cargo.toml

The diff converts every inline array (keywords = ["a", "b"]) to multiline format. This changes 40+ lines with zero functional impact, inflating the diff and making the actual changes harder to review.

Please revert all formatting-only changes and keep only the lines that add new dependencies or features (console_error_panic_hook, js-sys, crate-type).

4. New external dependency without justification

File: crates/office2pdf/Cargo.tomlconsole_error_panic_hook = { version = "0.1", optional = true }

Project rules (CLAUDE.md): "Always prefer Rust native implementations. Only use third-party crates when there is a clear, justified need."

console_error_panic_hook is a tiny crate (just calls std::panic::set_hook with web_sys::console::error), but the PR doesn't explain why a manual 5-line set_hook + web_sys::console::error_1 wouldn't suffice, or why web_sys (already transitively depended on via typst) can't be used directly.

Either justify the dependency or replace with an inline implementation.


MEDIUM

5. WASM timing always returns Duration::ZERO — metrics are silently broken

File: crates/office2pdf/src/lib.rs

#[cfg(target_arch = "wasm32")]
type TimingMark = ();

#[cfg(target_arch = "wasm32")]
fn start_timing() -> TimingMark {}

#[cfg(target_arch = "wasm32")]
fn elapsed_since(_start: TimingMark) -> Duration {
    Duration::ZERO
}

On WASM, ConvertMetrics will report parse_duration = 0, codegen_duration = 0, compile_duration = 0, total_duration = 0 for every conversion. This is misleading — users calling convert_bytes() on WASM and inspecting metrics get zeroes with no indication they're fake.

Options:

  • Use js_sys::Date::now() (already added as a dependency) for actual millisecond-precision timing on WASM.
  • Or make ConvertMetrics optional (metrics: None) on WASM so callers know timing is unavailable.

Zero durations that aren't actually zero is the worst option — it's silent data corruption.

6. catch_unwind added around render pipeline without explanation

File: crates/office2pdf/src/lib.rs (Stage 2+3 block)

The render pipeline (typst codegen + PDF compilation) was intentionally not wrapped in catch_unwind — only the parsing stage was, because upstream parsers (docx-rs, umya-spreadsheet) are known to panic on malformed input. Typst's compilation pipeline is not expected to panic.

Adding catch_unwind here:

  • Masks potential bugs in our own codegen (panics should propagate during development so we fix them)
  • Has a (small) performance cost
  • Changes the error contract (previously: codegen panics crash the process; now: they're silently converted to ConvertError::Render)

If WASM requires this because wasm-bindgen can't handle unwinding, please explain that explicitly in a code comment. If it's just "defensive programming", it should be removed — we prefer to fix panics rather than catch them.

7. WASM always includes PDF timestamp, native does not

File: crates/office2pdf/src/render/pdf.rs

#[cfg(target_arch = "wasm32")]
let timestamp = Some(typst_pdf::Timestamp::new_utc(current_utc_datetime()));

#[cfg(not(target_arch = "wasm32"))]
let timestamp = {
    let needs_timestamp = pdf_standard.is_some() || pdf_ua;
    // ...
};

On native, timestamp is only included for PDF/A and PDF/UA. On WASM, it's always included. This behavioral divergence is not documented. Why should WASM PDFs always have timestamps? If Typst requires it on WASM for some reason, add a comment explaining why.


LOW

8. Demo WASM path is fragile

File: crates/office2pdf/examples/wasm-web/app.js

const resolvedWasmUrl = new URL("../../pkg/office2pdf_bg.wasm", import.meta.url);

This assumes wasm-pack build --target web was run from crates/office2pdf/ and output to crates/office2pdf/pkg/. The relative path ../../pkg/ will break if:

  • The demo is served from a different directory
  • wasm-pack output path changes
  • Someone copies the demo elsewhere

Consider making the path configurable or at minimum add a comment explaining the expected directory layout.

9. README changes should be scoped to the example

The WASM demo instructions (wasm-pack build, python3 -m http.server) are useful but belong in crates/office2pdf/examples/wasm-web/README.md, not the project root README. The main README should link to the example directory, not contain step-by-step demo setup instructions.


Summary

# Severity Issue
1 CRITICAL cdylib crate-type affects all native builds
2 CRITICAL Indentation error in streaming metrics
3 HIGH Unnecessary Cargo.toml reformatting (40+ noise lines)
4 HIGH External dependency added without justification
5 MEDIUM WASM metrics silently return zero
6 MEDIUM catch_unwind on render pipeline without justification
7 MEDIUM Behavioral divergence: WASM always includes timestamp
8 LOW Fragile relative WASM path
9 LOW Demo instructions in root README

Please address at least the CRITICAL and HIGH items. Happy to re-review once updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants