From be20841fd3c7020fd329b29d2c55d8fea01146fa Mon Sep 17 00:00:00 2001 From: productdevbook Date: Thu, 13 Aug 2026 18:30:20 +0200 Subject: [PATCH] refactor: two names that meant different things in different files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither is a bug today. Both are the shape a bug arrives in: a line moved between two files keeps working and quietly changes contract. **`parseCellRef`** is exported from `xlsx/worksheet.ts` and is deliberately lenient — a cell reference that will not parse should cost one cell, not the read. `pivot-writer.ts` had a private one under the same name that *throws*, which is right for what it does (a `targetCell` the caller typed, where a typo has no salvageable reading and pivoting at A1 silently would be worse) and wrong to reach for anywhere else. It is now `parseCellRefStrict`, with the reason for the difference on it. **`decodeUtf8`** means "decode, and report a part over V8's `MAX_STRING_LENGTH` as the #514 `ParseError` naming it" in `reader.ts`, `ods/reader.ts` and `xlsb/reader.ts`, and "decode" in `stream-reader.ts` and `ods/stream.ts`. The streaming readers not having the ceiling is intentional and is what `docs/PARITY.md` records — `streamXlsxRows` is the answer *to* the ceiling, since it SAX-parses the worksheet off the decompression stream and never builds one string for it. The two spellings are now `decodeUtf8` and `decodeUtf8Unchecked`, so the difference is visible at the call site rather than only in PARITY. Both notes record what is still true of each streaming path: the parts `stream-reader.ts` decodes whole are the small ones, so a `sharedStrings.xml` over the ceiling would throw V8's raw error; and `streamOdsRows` streams rows out of `content.xml` but still builds the whole part as a string to do it. Names only. `pnpm test` green — 10,583 tests, 234 files. Co-Authored-By: Claude Opus 5 (1M context) --- src/ods/stream.ts | 16 ++++++++++++++-- src/xlsx/pivot-writer.ts | 15 ++++++++++++--- src/xlsx/stream-reader.ts | 40 ++++++++++++++++++++++++++------------- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/src/ods/stream.ts b/src/ods/stream.ts index 50ea63ef..80185a7b 100644 --- a/src/ods/stream.ts +++ b/src/ods/stream.ts @@ -11,7 +11,19 @@ import { MAX_COL_INDEX, MAX_REPEAT_COUNT, MAX_ROW_INDEX } from "../limits" // ── Helpers ────────────────────────────────────────────────────────── -function decodeUtf8(data: Uint8Array): string { +/** + * Decode a part with no string-length ceiling — see the fuller note on + * the same function in `xlsx/stream-reader.ts`. `ods/reader.ts` has a + * `decodeUtf8` that does check and raises the #514 `ParseError`; this + * one does not, and the name says so rather than leaving two functions + * with one name and two contracts. + * + * `streamOdsRows` streams the *rows* out of `content.xml` but still + * builds the whole part as a string to do it, so an ODS over the ceiling + * fails here with V8's raw error — unlike `streamXlsxRows`, which SAX-parses + * the worksheet off the decompression stream and never builds one. + */ +function decodeUtf8Unchecked(data: Uint8Array): string { return new TextDecoder("utf-8").decode(data) } @@ -331,7 +343,7 @@ export async function* streamOdsRows( if (!zip.has("content.xml")) { throw new ParseError("Invalid ODS: missing content.xml") } - const contentXml = decodeUtf8(await zip.extract("content.xml")) + const contentXml = decodeUtf8Unchecked(await zip.extract("content.xml")) // 4. Yield rows via SAX // 4. Yield rows via SAX, applying the filters diff --git a/src/xlsx/pivot-writer.ts b/src/xlsx/pivot-writer.ts index d9f8c044..d756dec3 100644 --- a/src/xlsx/pivot-writer.ts +++ b/src/xlsx/pivot-writer.ts @@ -623,7 +623,7 @@ function computeLocation( colFieldCount: number, meta: PivotFieldsMeta, ): Record { - const { col, row } = parseCellRef(targetCell) + const { col, row } = parseCellRefStrict(targetCell) // Header rows: 1 (page filters) + 1 (column field header per col field) + 1 (data field header) const firstHeaderRow = 0 @@ -644,8 +644,17 @@ function computeLocation( } } -/** Parse `"B3"` → `{col: 1, row: 2}` (0-based). */ -function parseCellRef(cell: string): { col: number; row: number } { +/** + * Parse `"B3"` → `{col: 1, row: 2}` (0-based), rejecting anything else. + * + * Deliberately not `xlsx/worksheet.ts`'s `parseCellRef`, which is lenient + * because a cell that mis-parses in a worksheet should cost one cell, not + * the read. This one takes a `targetCell` the caller wrote by hand, where + * a typo has no salvageable reading and silently pivoting at A1 would be + * worse than the throw. The name says `Strict` so the two are not + * mistaken for each other. + */ +function parseCellRefStrict(cell: string): { col: number; row: number } { const m = /^([A-Z]+)(\d+)$/i.exec(cell.trim()) if (!m) { throw new Error(`Invalid pivot targetCell "${cell}" — expected an A1-style reference`) diff --git a/src/xlsx/stream-reader.ts b/src/xlsx/stream-reader.ts index b473ad0b..8f89e03a 100644 --- a/src/xlsx/stream-reader.ts +++ b/src/xlsx/stream-reader.ts @@ -68,7 +68,21 @@ const REL_STYLES = "styles" // ── Helpers ────────────────────────────────────────────────────────── -function decodeUtf8(data: Uint8Array): string { +/** + * Decode a package part with no string-length ceiling. + * + * The buffered readers route the same job through `_decode.decodePart`, + * which catches V8's `MAX_STRING_LENGTH` and reports it as the #514 + * `ParseError` naming the part. This path does not, and that is what + * `docs/PARITY.md` records — `streamXlsxRows` is the answer *to* the + * ceiling for worksheets, since it never builds one string for them. + * + * The parts decoded here are the small ones (content types, rels, the + * workbook, styles); a `sharedStrings.xml` over the ceiling would still + * throw V8's raw error. The name is `Unchecked` so a line moved between + * the two readers cannot quietly drop the check. + */ +function decodeUtf8Unchecked(data: Uint8Array): string { return new TextDecoder("utf-8").decode(data) } @@ -645,9 +659,9 @@ function resolveFromParts( const ct = parts.get("[Content_Types].xml") const rootRelsBytes = parts.get("_rels/.rels") if (!ct || !rootRelsBytes) return null - parseContentTypes(decodeUtf8(ct)) + parseContentTypes(decodeUtf8Unchecked(ct)) - const rootRels = parseRelationships(decodeUtf8(rootRelsBytes)) + const rootRels = parseRelationships(decodeUtf8Unchecked(rootRelsBytes)) const workbookRel = rootRels.find((r) => matchesRelType(r.type, REL_WORKBOOK)) if (!workbookRel) return null const workbookPath = workbookRel.target.startsWith("/") @@ -662,9 +676,9 @@ function resolveFromParts( ? `${workbookDir}/_rels/${workbookPath.slice(workbookDir.length + 1)}.rels` : `_rels/${workbookPath}.rels` const wbRelsBytes = parts.get(workbookRelsPath) - const workbookRels = wbRelsBytes ? parseRelationships(decodeUtf8(wbRelsBytes)) : [] + const workbookRels = wbRelsBytes ? parseRelationships(decodeUtf8Unchecked(wbRelsBytes)) : [] - const { sheets: sheetInfos, dateSystem } = parseWorkbookXml(decodeUtf8(wbBytes), options) + const { sheets: sheetInfos, dateSystem } = parseWorkbookXml(decodeUtf8Unchecked(wbBytes), options) const targetSheet = resolveTargetSheet(sheetInfos, options?.sheet) if (!targetSheet) return null @@ -692,7 +706,7 @@ function resolveFromParts( const ssPath = resolvePath(workbookDir, ssRel.target) const ssBytes = parts.get(ssPath) if (!ssBytes) return null - sharedStrings = parseSharedStrings(decodeUtf8(ssBytes)) + sharedStrings = parseSharedStrings(decodeUtf8Unchecked(ssBytes)) } let parsedStyles: ParsedStyles | null = null @@ -701,7 +715,7 @@ function resolveFromParts( const stylesPath = resolvePath(workbookDir, stylesRel.target) const stylesBytes = parts.get(stylesPath) if (!stylesBytes) return null - parsedStyles = parseStyles(decodeUtf8(stylesBytes)) + parsedStyles = parseStyles(decodeUtf8Unchecked(stylesBytes)) } return { wsPath, sharedStrings, parsedStyles, dateSystem } @@ -829,14 +843,14 @@ export async function* streamXlsxRows( if (!zip.has("[Content_Types].xml")) { throw new ParseError("Invalid XLSX: missing [Content_Types].xml") } - const contentTypesXml = decodeUtf8(await zip.extract("[Content_Types].xml")) + const contentTypesXml = decodeUtf8Unchecked(await zip.extract("[Content_Types].xml")) parseContentTypes(contentTypesXml) // 3. Parse _rels/.rels to find the workbook path if (!zip.has("_rels/.rels")) { throw new ParseError("Invalid XLSX: missing _rels/.rels") } - const rootRelsXml = decodeUtf8(await zip.extract("_rels/.rels")) + const rootRelsXml = decodeUtf8Unchecked(await zip.extract("_rels/.rels")) const rootRels = parseRelationships(rootRelsXml) const workbookRel = rootRels.find((r) => matchesRelType(r.type, REL_WORKBOOK)) if (!workbookRel) { @@ -855,7 +869,7 @@ export async function* streamXlsxRows( let workbookRels: Relationship[] = [] if (zip.has(workbookRelsPath)) { - const wbRelsXml = decodeUtf8(await zip.extract(workbookRelsPath)) + const wbRelsXml = decodeUtf8Unchecked(await zip.extract(workbookRelsPath)) workbookRels = parseRelationships(wbRelsXml) } @@ -863,7 +877,7 @@ export async function* streamXlsxRows( if (!zip.has(workbookPath)) { throw new ParseError(`Invalid XLSX: missing workbook at ${workbookPath}`) } - const workbookXml = decodeUtf8(await zip.extract(workbookPath)) + const workbookXml = decodeUtf8Unchecked(await zip.extract(workbookPath)) const { sheets: sheetInfos, dateSystem } = parseWorkbookXml(workbookXml, options) // 6. Parse shared strings (small, needed for cell resolution) @@ -872,7 +886,7 @@ export async function* streamXlsxRows( if (ssRel) { const ssPath = resolvePath(workbookDir, ssRel.target) if (zip.has(ssPath)) { - const ssXml = decodeUtf8(await zip.extract(ssPath)) + const ssXml = decodeUtf8Unchecked(await zip.extract(ssPath)) sharedStrings = parseSharedStrings(ssXml) } } @@ -883,7 +897,7 @@ export async function* streamXlsxRows( if (stylesRel) { const stylesPath = resolvePath(workbookDir, stylesRel.target) if (zip.has(stylesPath)) { - const stylesXml = decodeUtf8(await zip.extract(stylesPath)) + const stylesXml = decodeUtf8Unchecked(await zip.extract(stylesPath)) parsedStyles = parseStyles(stylesXml) } }