feat(wasm-workbook): expose removeName on the JsWorkbook binding - #976
Merged
Conversation
`Workbook::remove_name` has existed in Rust since named-range CRUD landed, but the wasm binding only exposed `defineName`/`redefineName` — a JS consumer could define and redefine a name but never remove one. Deleting a named range from a JS-facing document model left the engine still resolving the old name: a stale value with no error, until a full rebuild forced a resync. `removeName(name)` delegates to `Workbook::remove_name`, which returns `Option<NamedRange>` and never fails (removing an unknown name is an intentional no-op), so the binding returns nothing rather than forcing an artificial `Result<(), JsError>` with a dead error arm — matching `clear()`'s existing silent-no-op convention rather than `defineName`/`redefineName`'s (which really can fail). The generated `.d.ts` still types it `removeName(name: string): void`, identical in shape to its two siblings. Adds `crates/wasm-workbook/tests/wasm_surface.rs`, gated to `wasm32` and run via `wasm-pack test --node`, mirroring `crates/wasm/tests/wasm_surface.rs`. It defines a name, uses it in a formula, removes it through the actual binding, recalcs, and asserts the formula now resolves to `#NAME?` instead of the stale value. Confirmed the test does not compile against the pre-fix binding (no such method) and passes after. closes #973 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HAt4Keq1m4fEyHPmPjHSJ7
Contributor
Test Coverage by Category
✓ = 100% passing · ⚠ = known deviation · The ~79,711 total counts formula evaluations (each conformance row and each property case = 1). GitHub Checks reports 4,119 Rust test functions: 3,062 unit + 159 property functions (shown as cases above) + 898 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.
Workbook::remove_name(crates/workbook/src/mutate.rs) is public and has always worked. The wasm binding incrates/wasm-workbook/src/lib.rsexposeddefineNameandredefineNamebut notremoveName— a JS consumer could define and redefine a named range but never remove one. Deleting a named range from a JS-facing document model left the engine still resolving the old name:=SUM(Total)kept returning the stale value after the range was deleted, only becoming#NAME?after a full rebuild forced a resync. A wrong value, silently, with no error.What changed
JsWorkbook::remove_name, exposed asremoveName(name), delegating straight toWorkbook::remove_name.Return-type note:
defineName/redefineNamereturnResult<(), JsError>because their Rust counterparts can fail (bad ref, dangling sheet, name collision, cap exceeded).remove_namereturnsOption<NamedRange>in Rust and cannot fail — removing an unknown name is an intentional no-op — soremoveNamereturns nothing rather than aResultwith a permanently-empty error arm, matchingclear()'s existing silent-no-op convention instead. The generated.d.tsstill types itremoveName(name: string): void, the identical shapedefineName/redefineNameget (they're alsovoidin TS despite being fallible —wasm-bindgenthrows onErrrather than surfacing it in the return type).This is a deliberate, not accidental, choice: if
remove_nameever grows a failure mode, changingremoveName's signature to return something is an ABI break for any caller that isn't already wrapping it in atry/catch. Flagging it here so it's a documented tradeoff rather than a surprise later.Also updated the one line in
crates/wasm-workbook/README.jsr.mdthat listed the named-range trio, to includeremoveName. The non-JSRcrates/wasm-workbook/README.mddocumentsdefineNamebut never documentedredefineNameeither — that's a pre-existing gap, not something this PR introduces, so it's left alone.Test
crates/wasm-workbook/tests/wasm_surface.rs(new), gated#![cfg(target_arch = "wasm32")]and run viawasm-pack test --node crates/wasm-workbook, mirroring the existingcrates/wasm/tests/wasm_surface.rspattern (needed because aJsWorkbookmethod touchingJsValue/JsErroraborts the process when called natively outside a real wasm runtime — confirmed while writing this, see report). It defines a name, uses it in=SUM(Total), asserts it resolves to30, callsremoveNamethrough the actual binding, recalcs, and asserts the formula now resolves to#NAME?.E0599: no method named 'remove_name' found for struct 'JsWorkbook') — a stronger signal than a runtime assertion failure, and independently reproduced during review.test remove_name_through_binding_invalidates_dependent_formula ... ok.Also checked: the same asymmetry elsewhere
Enumerated every public mutator on
Workbook(workbook.rs,mutate.rs,recalc.rs) against the wasm binding surface (crates/wasm-workbook/src/lib.rs). Bound:new,from_json→fromJSON,to_json→toJSON,add_sheet→addSheet,set→set,set(date)→setDate,clear→clear,define_name→defineName,redefine_name→redefineName,remove_name→removeName(this PR),define_table→defineTable,redefine_table→redefineTable,recalc→recalc.Missing from the binding (not fixed here — listing only, per scope):
remove_table(mutate.rs) — the exact same trio gap this PR fixes, one level over:defineTable/redefineTableare bound,removeTableis not.insert_sheet,remove_sheet,rename_sheet,move_sheet(workbook.rs) — onlyadd_sheetof the five sheet-lifecycle mutators is bound.recalc_incremental(recalc.rs) — only the fullrecalcis bound; incremental recalc (given an edited-cell list) has no JS-facing entry point.Not counted as missing bindings (excluded deliberately):
sheets_mut/names_mut/tables_mutare raw&mut Vec<T>escape hatches that bypass the validation (define/removemethods enforce caps, dangling-ref checks, collision checks) — binding them directly would be a regression, not a fix.drop_derived_stateonly invalidates the cache, no document content changes.trace_cellandseed_spill_sensitive_built_indextake&self, not mutators despite living inmutate.rs/recalc.rs.This PR adds only
removeName, per the issue's scope.Breaking change?
No — purely additive.
removeNameis a new method; nothing existing changes shape.Verify
cargo build --locked --workspace --exclude truecalc-python— clean. (truecalc-pythonfails to link locally on this machine for an unrelated pre-existing reason — missing systemlibpython3.9— not something this change touches; CI'scargo clippy --workspace -- -D warningsstep builds the full workspace including it.)cargo test --workspace --exclude truecalc-python— 4127 passed (112 suites).cargo clippy --workspace --exclude truecalc-python -- -D warnings— no issues.wasm-pack build crates/wasm-workbook --target web— succeeds; generated.d.tsshowsremoveName(name: string): voidalongsidedefineName/redefineName.wasm-pack test --node crates/wasm-workbook— new test passes.closes #973
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.