Skip to content

Fix #2943: preserve falsy formula results (0, false, empty string) in copy#52

Closed
senoff wants to merge 1 commit intoprotobi:masterfrom
senoff:fix-2943-formulavalue-zero
Closed

Fix #2943: preserve falsy formula results (0, false, empty string) in copy#52
senoff wants to merge 1 commit intoprotobi:masterfrom
senoff:fix-2943-formulavalue-zero

Conversation

@senoff
Copy link
Copy Markdown

@senoff senoff commented May 1, 2026

Summary

Fixes exceljs/exceljs#2943: copying a formula cell whose cached result is 0, false, or '' produced a destination cell with no result.

The root cause is FormulaValue._copyModel in lib/doc/cell.js, which iterates over candidate fields with if (value) — a truthy check that drops every falsy result. Because _copyModel backs both the value getter and setter, the bug fires on the canonical copy idiom dest.value = src.value (and on every plain read of cell.value on a formula cell with a falsy cached result).

While auditing the same file I found three sibling falsy-check bugs and one xlsx-reader bug with the same shape, all fixed in this PR.

Changes

lib/doc/cell.jsFormulaValue

  • _copyModel: replace if (value) with if (name in model && model[name] !== undefined) so 0, false, '' survive the copy.
  • toCsvString: replace ${this.model.result || ''} with an explicit null/undefined check; previously result === 0 rendered as empty string in CSV.
  • toString: replace this.model.result ? ... : '' ternary with the same explicit check.
  • effectiveType: add a typeof v === 'boolean' branch so a formula with result: false returns ValueType.Boolean instead of throwing on false.text when falling through to the hyperlink check.

lib/xlsx/xform/sheet/cell-xform.jsparseClose

  • Replace if (model.value) with if (model.value !== undefined) and add an else if (this.t === 'str') branch that sets model.result = '' for <c t="str"><f>...</f><v/></c>. Without this, a string-typed formula with empty-string result drops the result on read.
  • Drive-by: extract the long shared-formula error message to a local so prettier doesn't restructure it across lines (avoids a pre-existing prettier vs. eslint comma-dangle conflict that fires whenever this file is edited).

Tests

spec/integration/issues/issue-2943-formula-falsy-result.spec.js — 9 assertions:

  • value-to-value copy preserves result === 0, false, ''
  • xlsx round-trip preserves result === 0, false, ''
  • toCsvString returns '0' for result === 0 and 'false' for result === false
  • toString returns '0' for result === 0

All pre-existing tests pass: 884 unit + 209 integration + 1 end-to-end.

Test plan

  • npm run test:unit — 884 passing
  • npm run test:integration — 209 passing (was 207; +2 from new spec)
  • npm run test:end-to-end — 1 passing
  • eslint clean on changed files
  • prettier --check clean on changed files
  • Pre-commit hook (lint-staged) succeeds without --no-verify

…ing) in copy

The FormulaValue copy path used truthy checks (`if (value)`) that dropped
formula results equal to 0, false, or ''. Copying a cell with cached
result === 0 produced a destination cell with no result.

Fixes:
- lib/doc/cell.js FormulaValue._copyModel: replace `if (value)` with
  `if (name in model && model[name] !== undefined)` so falsy results
  survive value getter/setter copy.
- lib/doc/cell.js FormulaValue.toCsvString / toString: stop collapsing
  falsy results to '' via `||` / ternary; only treat null/undefined as
  absent.
- lib/doc/cell.js FormulaValue.effectiveType: add Boolean branch so a
  result === false returns ValueType.Boolean instead of crashing on
  `false.text`.
- lib/xlsx/xform/sheet/cell-xform.js parseClose: replace `if (model.value)`
  with `if (model.value !== undefined)` and special-case `<v/>` for
  `t="str"` formula cells so empty-string results survive xlsx round-trip.

Drive-by: extract long shared-formula error message to a local so prettier
doesn't restructure the throw across lines (avoids prettier/eslint
trailing-comma conflict on edits to this file).

Standalone Mocha regression at spec/integration/issues covers value-copy
and xlsx round-trip for all three falsy results plus toCsvString/toString.

Refs exceljs#2943.
@senoff
Copy link
Copy Markdown
Author

senoff commented May 3, 2026

Local test run on this branch — npm install --legacy-peer-deps && npm run test:unit && npm run test:integration:

Tier This PR Baseline (master @ 4c9e73c) Δ
test:unit 884 passing, 1 pending, 0 failing 884 passing, 1 pending, 0 failing +9 int (this PR adds tests)
test:integration 209 passing, 0 failing 200 passing, 0 failing +9 int (this PR adds tests)

No regressions vs baseline. Skipped test:end-to-end + test:jasmine (browser/grunt tiers) — same regression-catching surface for unit + integration on a feature branch like this one.

Posted because the tests.yml workflow at this repo hasn't auto-fired on this PR (GitHub's first-time-contributor approval gate). Just want to make the merge feel less risky if/when you get to it. Happy to wait for the official CI run if you'd prefer to flip the approval gate; this is purely supplementary.

@senoff
Copy link
Copy Markdown
Author

senoff commented May 4, 2026

Ran exceljs's headless test suite against this PR:

test:unit         884 passing
test:integration  209 passing
test:end-to-end   1 passing
                 ─────────────
                 1094 passing, 0 failing

(Skipped test:jasmine — browser harness not set up locally.)

All green. Hopefully useful evidence for review.

@senoff
Copy link
Copy Markdown
Author

senoff commented May 7, 2026

Regenerated per AGENTS.md guidance — see new PR #68. Closing this one in favor of the regenerated version.

@senoff senoff closed this May 7, 2026
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.

Misshandling of value 0 (zero) in copy operation in FormulaValue

1 participant