Repository navigation
feat(wasm-workbook): expose a typed cell-value method alongside the existing JsValue one - #997
Merged
Merged
Conversation
…string get/resolved Extracts EvalResult (and the Value -> EvalResult mapping) out of crates/wasm into a new internal crate, truecalc-wasm-value, so @truecalc/core and @truecalc/workbook share one tagged-value shape instead of each hand-rolling it. crates/wasm re-exports the type unchanged; crates/wasm-workbook adds its own arm-by-arm mapping from truecalc_workbook::Value (a distinct type from truecalc_core::Value). wasm-workbook cannot depend on wasm directly to reuse EvalResult: wasm-bindgen keeps every #[wasm_bindgen] free function as an export root regardless of whether the consuming crate calls it, so linking wasm as a library would leak wasm's own evaluate/validate/createEngine/ etc. into wasm-workbook's compiled artifact and public API (verified via wasm-objdump export-table diff). A shared, wasm-bindgen-free crate avoids that. JsWorkbook gains getTyped/resolvedTyped, returning the real EvalResult marshaled across the WASM ABI via tsify instead of the JSON string get/resolved return today. get/resolved are byte-for-byte unchanged (same JsValue/JSON-string return, doc comment only) so nothing currently JSON.parse()-ing them breaks; the new methods are a purely additive surface for callers to migrate to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
…f added The prior commit's review concluded the repo's fmt CI check was a pre-existing, unrelated failure and not a blocker. That conclusion was wrong for this PR's own new code: .github/scripts/fmt-changed.sh is a per-file ratchet, not a whole-workspace check — it grandfathers files that were already fmt-dirty at the merge base (crates/wasm/src/lib.rs qualifies) but enforces fmt unconditionally on added files and on files that were previously clean. crates/wasm-value/src/lib.rs is a brand-new file and crates/wasm-workbook/src/lib.rs was clean at origin/main, so the local rustfmt's struct-literal-expansion style applies to both and the script fails on exactly the new EvalResult enum and the two new value_to_eval_result match arms this feature added. Verified by actually running `bash .github/scripts/fmt-changed.sh origin/main` before and after. Ran `--fix` scoped to just the two failing files (not the grandfathered crates/wasm/src/lib.rs, to avoid reformatting unrelated pre-existing code) and confirmed the ratchet script now reports "No new formatting drift." Full local CI suite re-run clean: cargo clippy -D warnings (0 issues), cargo test --workspace --exclude truecalc-python (4216 passed), cargo build --target wasm32-unknown-unknown for both wasm and wasm-workbook, and wasm-pack test --node for both crates (4 + 2 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WfhRbND7tjFJ4JeGZ9gHL5
Contributor
Test Coverage by Category
✓ = 100% passing · ⚠ = known deviation · The ~79,739 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,207 Rust test functions: 3,090 unit + 159 property functions (shown as cases above) + 958 conformance/integration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this closes
@truecalc/core'sEvalResult(the discriminated-union{ type: "number", value: ... }shape) has always been the WASM surface's canonical value representation, but@truecalc/workbook'sget()/resolved()only ever exposed it serialized to a JSON string that callers had toJSON.parse()themselves — with no static type on the other end. Types flowed left (Rust) to nowhere useful on the right (the JS/TS consumer had to re-derive the shape by hand or trust a hand-written.d.ts).This PR closes that gap:
get()/resolved()gain typed counterparts,getTyped()/resolvedTyped(), that marshal a realEvalResultacross the WASM ABI viatsifyinstead of returning a JSON string — so the type flows all the way from the RustValueenum to a real generated TypeScript type on the consumer's side, left to right, with no manual re-parsing or hand-maintained shape declarations in between.What changed
New internal crate
truecalc-wasm-value(crates/wasm-value,publish = false,rlibonly): hostsEvalResult,SparklineSpecResult, and theValue -> EvalResultmapping that used to live only incrates/wasm/src/lib.rs.crates/wasmnow re-exports these instead of defining them, so@truecalc/core's public API is untouched (see verification below).crates/wasm-workbook: addsvalue_to_eval_result(an arm-by-arm mapping fromtruecalc_workbook::Value, a distinct type fromtruecalc_core::Value, onto the sharedEvalResult), plus two new#[wasm_bindgen]methods:getTyped(sheet, a1) -> GetResult | undefined— typed counterpart ofget()resolvedTyped(sheet, a1) -> ResolvedResult | undefined— typed counterpart ofresolved(),anchorincludedBoth return
undefined(notget/resolved's JSONnull) for a missing cell — the idiomatic optional shape for a typed WASM return.get()andresolved()themselves are completely unchanged — same JSON-string return, same shape, same behavior. This is purely additive.Why it's additive / non-breaking
Verified two ways, not just asserted:
@truecalc/core's generated.d.tsis byte-identical except for one added doc-comment sentence. I builtcrates/wasmwithwasm-pack build --target webat bothorigin/mainand this branch's tip and diffed the twotruecalc_wasm.d.tsfiles — the only change is a sentence added toEvalResult's doc comment explaining it's now shared with@truecalc/workbook. No type, field, or method changed.@truecalc/workbook's generated.d.tsdiff is purely additive. Same before/after build comparison forcrates/wasm-workbook: every existing type, method signature, and exported WASM binding is untouched. The diff adds only the newEvalResult/SparklineSpecResult/GetResult/ResolvedResulttypes and the two newgetTyped/resolvedTypedmethods (plus their corresponding newjsworkbook_getTyped/jsworkbook_resolvedTypedlow-level WASM exports).Runtime verification (real cell values, before vs after)
Built both
origin/mainand this branch'scrates/wasm-workbookfor thewebtarget and drove both through Node (viainitSyncwith the raw.wasmbytes) against a workbook with a number, text, bool, a#DIV/0!error, and a spilling={1,2;3,4}array formula:get()/resolved()output is byte-for-byte identical before and after this PR on every cell (confirms zero behavior change to the untouched methods).getTyped()/resolvedTyped()matchJSON.parse(get()).value/JSON.parse(resolved()).valuefield-for-field for every scalar and the error case (key-order differences aside, which don't apply to real typed objects).get()'s existing JSON shape nests raw JS arrays ([[1,2],[3,4]]);getTyped()returns the documented recursivearray-of-arrayEvalResultshape ({type:"array",value:[{type:"array",value:[...]}]}) — consistent with every other value kind instead of a special-cased raw array. This is by design, documented inEvalResult's own doc comment, and confirmed working for the spill-anchor and spilled-cell (anchorfield) cases.Test coverage
crates/wasm-workbook/tests/typed_value.rs(new, 15 tests, nativecargo test): exhaustive per-Value-variant coverage forgetTyped/resolvedTyped— number, text, bool, date, zoned, error (with and without diagnostic message), empty, sparkline, and the 2-D spill-anchor/spilled-cell array cases — built viatruecalc_workbookdirectly and loaded throughJsWorkbook::fromJSONsince several of these variants have no JS-levelset()literal form.crates/wasm-workbook/tests/wasm_surface.rs(extended, 1 newwasm_bindgen_test): the one thing the native tests above can't exercise — marshaling through the real#[wasm_bindgen]-generated ABI wrapper, not just the inherent Rust method.Verification run independently for this PR
cargo fmt --all -- --check: pre-existing repo-wide drift unrelated to this diff (2453 hunks across untouched files, confirmed by diffing against files this PR doesn't touch); every file this PR adds or touches is fmt-clean on its own. CI'sfmtjob is a ratchet on changed files only, which this satisfies.cargo clippy --workspace -- -D warnings: clean.cargo test --workspace --exclude truecalc-python: all green (truecalc-pythonfails to link locally only because this machine is missing a Python 3.9 dev library — a pre-existing local-environment gap, not something this PR touches; CI's ownTeststep excludes the same crate for an unrelated, permanent reason — itsextension-modulebuild can't link libpython in a test binary).cargo deny check: clean.wasm-pack build crates/wasm --target webandwasm-pack build crates/wasm-workbook --target web: both build clean.For the reviewer
The diff touches 4 crates but the actual new logic is small: one new tagged-value-to-
EvalResultmapping function (value_to_eval_resultincrates/wasm-workbook/src/lib.rs) and two thin#[wasm_bindgen]wrapper methods around existinginner.get/inner.resolvedcalls. Thetruecalc-wasm-valuecrate split is a pure code-motion (cut fromcrates/wasm/src/lib.rs, pasted with no logic changes) to let both WASM packages share oneEvalResultdefinition instead of hand-rolling two copies of the same tagged union.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.