Repository navigation
Refactor/parser context extraction - #112
Merged
Merged
Conversation
Architecture epic #4, Stage 3 — the closure-soup-to-typed-context migration the boundary planner flagged as the riskiest stage. Replaces ~50 \`let\` declarations across the top of parseZPL with a single typed \`ParserState\` object passed through the body as \`s\`. Every primitive state read/write becomes \`s.foo\` / \`s.foo = …\`; collection refs (objects, labelConfig, printerProfile, variables, findings sinks) destructure as bare names for terseness since their mutations are method-call shaped. Mechanically migrated via a one-shot \`scripts/migrate_parser_state.local.py\` that prefixed all 60 state-var references with \`s.\` using word-boundary regex with a negative-lookbehind for \`.\` (so existing property accesses like \`modelPos.x\` and \`pendingReverseBg.y\` stay untouched). Three manual fixups: the destructure block at parseZPL entry, the \`pendingReverseBg = { x, y, … }\` shorthand literal (now explicit \`x: s.x, y: s.y\`), and the import-cleanup pass (dropped 5 unused type imports that the closure-let removal made redundant). New module: src/lib/zplParser/context.ts (210 LOC) — ParserState interface, PendingReverseBg interface, REVERSE_BBOX_TOLERANCE_DOTS, createParserState() factory. zplParser.ts shrinks 1757 → 1592 (−165 LOC). Combined with the prior Stage 1-2 extractions (decoders, helpers, types) the parser subtree is now 8 focused modules; what's left in zplParser.ts is the orchestrator + closure helpers + handler registry — none of which depend on closure-captured state anymore. Unlocks the next epic-#4 step: extracting the barcodes / graphics / fields handler families as \`create{Family}(state)\` factories, same pattern as setupScript / labelConfig / unsupported already landed. Net diff: 1409 tests pass (zero behaviour change verified by the existing suite + the round-trip + tripwire coverage). Lint 0 errors, build green.
Architecture epic #4, Stage 4 — the three remaining handler families extracted from parseZPL.ts using the family pattern established in the cleanup-PR (setupScript / labelConfig / unsupported). Each family file exports a `create{Family}Handlers(state, …deps)` factory returning a `Record<string, Handler>` and gets merged into the dispatch map via `Object.assign(handlers, createX(s, …))`. Three families: - `src/lib/zplParser/handlers/barcodes.ts` (164 LOC) — `mkBarcode` factory + ^BC, ^BE, ^BU, ^B3, ^BA, ^B1, ^B2, ^BI, ^BJ, ^BK, ^BL, ^BP, ^BZ, ^BS, ^BM (MSI with check-letter logic), ^BR (GS1 Databar with byModuleWidth side-effect), ^BQ (QR), ^BX (DataMatrix), ^B7 (PDF417), ^B0 / ^BO (Aztec via shared `handleAztec`), ^BV (Maxicode), ^BF (MicroPDF417), ^BB (CODABLOCK), plus ^BY barcode defaults that the rest of the family inherits via state. - `src/lib/zplParser/handlers/graphics.ts` (503 LOC) — ^GB / ^GC / ^GD / ^GE / ^GF / ^GS / ^XG / ~DY. Owns the reverse-text-on-black collapse protocol: `pushGBObject` + `commitPendingReverseBg` + `getReverseFlag` are exported via `GraphicsFamily.helpers` so `flushField` (still in parseZPL.ts) can drive the collapse-or- commit decision on the next field. ~DY also owns the graphic-upload preamble that registers entries in `s.downloadedGraphics` for a later ^XG to recall, and the TTF/OTF font-bytes path that loads into `loadFontBytesSync`. - `src/lib/zplParser/handlers/fields.ts` (304 LOC) — ^FO / ^FT / ^A0 / ^A@ / ^TB / ^CF / ^FW / ^FB / ^FH / ^FD / ^FS / ^SN / ^SF / ^LR / ^FR / ^LH / ^LT / ^CW / ^FX / ^CI / ^FN / ^FC / ^FE. Takes `flushField` + `appendComment` via a `FieldHelpers` DI arg since both are owned by parseZPL.ts (`flushField` is the 445-LOC gravitational center, the next epic-#4 stage candidate). Plus post-extraction polish folded through three review rounds: - `readRotation` extracted to `src/lib/zplParser/helpers.ts` as a shared helper with an optional `fallback` param (defaults to 'N'). Was duplicated as inline closures in barcodes / graphics and as bare `(rest[0] as TextProps["rotation"]) ?? fwRotation` type-casts at four sites (parseZPL.ts dynamic ^A, fields.ts A0/A@/TB). HARDENING: the old cast was structural — invalid chars like "X" would slip through and land in state. The new helper validates via `isZplRotation` and falls back. Behaviour-preserving for every valid ZPL input; rejects previously-silently-stored garbage. - `getReverseFlag` deduplicated. parseZPL.ts had a one-liner copy of the closure that lives inside `createGraphicsHandlers`. Now exposed via `GraphicsFamily.helpers.getReverseFlag` so `flushField` destructures it from the same factory call that the graphics handlers close over. Single closure, single `s.lrActive` / `s.frActive` read path. - `IMPORT_FINDING_PAYLOAD_LIMIT` replaces a magic `80` literal at the DY hex-error inline-summary path. `dySummary` const reused at the same site instead of rebuilding the same template literal. - Stale `dpmm` JSDoc dropped from `createFieldHandlers` (the param wasn't on the signature). zplParser.ts: 1592 → 738 LOC (−854 / −54% in this stage; combined with Stage 3 the parser is 2466 → 738 LOC, −70% since the start of the epic). What stays in parseZPL.ts: orchestrator, `flushField`, `bootstrapVariable`, `applyFnEmbeds`, `takeComment`, `resetFB`, `resetComment`, `appendComment`, XA / XZ structural handlers, dynamic `^A{X}` font fallthrough, findings-assembly tail. 1409 tests pass; lint 0 errors; build green. No behaviour change across any of the 5 critical invariants (^CW alias resolution, ^CI → ^FH chain, ^BY → barcode defaults, ^FC / ^FE per-block reset, ^CF → ^A field-emit) and the cross-block printerProfile fold.
Architecture epic #4, flushField extraction. The 445-LOC field-emit closure (giant switch by `s.fieldType` that turns cached state into a pushed LabelObject) moves to `src/lib/zplParser/flushField.ts` behind a `createFlushField(s, deps) → () => void` factory. Three closure helpers move with it (only called from inside the flushField body): - `bootstrapVariable` — get-or-create the Variable for an ^FN slot - `applyFnEmbeds` — bootstrap embed-referenced FNs at flush time - (resetFB stays in parseZPL.ts — used by the ^FS handler in the fields family AND by the field-block reset paths) `takeComment` stays at parseZPL.ts level as a peer-shared closure (graphics family + flushField both need it). Idiomatic alternative to a build-order workaround: declare `takeComment` first, build graphics next (takes it), build flushField last (takes both `takeComment` and the graphics-family helpers as plain DI). zplParser.ts: 738 → 200 LOC (−538, the file is now almost entirely imports + orchestration + the structural ^XA/^XZ handlers + dynamic ^A{X} fallthrough + findings tail). flushField.ts: 562 LOC (446 LOC of the switch + 80 LOC of bootstrap- Variable / applyFnEmbeds + factory shape). Combined with Stage 3 + Stage 4, the parser has gone from 2466 LOC in one file to ~200 LOC orchestrator + 14 focused modules under `src/lib/zplParser/` (types, helpers, context, flushField, 3 decoder modules, 6 handler-family modules). The remaining stage candidate is per-type splitting of the flushField switch — that's where the ParserState sub-state grouping would naturally emerge (text-emit needs only the text-pending slice, etc). 1409 tests pass; lint 0 errors; build green.
The previously-flat 60-field ParserState is now organised into 8 typed sub-slices, each owning a coherent concept: - `result` (8 fields) — public output accumulators: objects, labelConfig, printerProfile, variables, plus the 4 findings sinks (skipped, partialCmds, browserLimit, unknown). - `label` (4 fields) — ^LH / ^LT / ^LR persistent label-frame state that applies to every following field until overridden. - `comment` (3 fields) — ^FX carry + ^FN slot machinery. pendingComment → comment.pending, pendingFn → comment.fnNumber, pendingFnComment → comment.fnComment. - `format` (5 fields) — ^FE / ^FC / ^FH / ^CI format-scoped template + decoding state. Reset at ^XA / ^XZ to per-format defaults. - `defaults` (10 fields) — ^CF / ^FW / ^FB / ^BY persistent defaults applied to following fields. ^FB is the only group that resets after each text field (handled inside flushField); the others persist until overridden. - `fonts` (3 fields) — ^CW alias map + ~DY upload tracking that spans the whole parse. fontAliases → fonts.aliases. - `reverseBg` (top-level, not nested) — ^GB stash for the reverse- text collapse. Stays flat because the value is null in the common case and the collapse is a one-field stash, not a sub-state. pendingReverseBg → reverseBg. - `field` (32 fields) — per-field accumulator: position (x, y, positionIsFT), type discriminator + ^FD payload, ^FR per-field reverse, plus 27 per-symbology pending params (text/barcode/ symbol/qr/dm/pdf/aztec/maxicode/mpdf/codablock/sn/font). Populated by handlers, consumed and reset by flushField at ^FS. Type-safety win: each handler family can now take just the slices it actually mutates, so a barcode handler accidentally writing `s.label.lhX` becomes a compile error instead of a silent correctness bug. Families currently still pass `s` as a whole because their constructors haven't been updated to narrow the slice — that's a follow-up refactor that lands per family without behaviour change. Migration via `scripts/migrate_parser_state_grouping.local.py` — mechanical word-boundary regex rewrite of `s.foo` → `s.<group>.foo` across 8 parser files (420 rewrite sites total: zplParser.ts 22, flushField.ts 168, barcodes.ts 61, graphics.ts 65, fields.ts 104; setupScript/labelConfig/unsupported.ts don't touch state directly so 0 each). The createParserState() factory in context.ts produces the matching nested literal. 1409 tests pass; lint 0 errors; build green. No behaviour change — the field-name accesses just move from flat to nested, no logic flips.
Picks up where e50546e (barcodes narrowed to `(field, defaults)`)
left off. Three review-driven polish items:
- **fields**: drop the redundant `labelConfig` parameter. It was
reachable via `s.result.labelConfig` already; passing it as a
separate arg was an artefact of the pre-grouping signature.
Family now takes `(s, helpers)` and pulls `labelConfig` from
`s.result` inline. Consistent with every other family's
`s.result.*` access path.
- **context.ts**: document the narrowing heuristic next to
`ParserState`. Rule: pass individual slices when the family
mutates ≤ 2 of them (barcodes → field+defaults, setupScript →
printerProfile, labelConfig → labelConfig); pass `s` whole when
≥ 3 slices, because the long parameter list duplicates the
ParserState interface without adding type-safety. Sub-slices
still enforce ownership at the access path even when the whole
`s` is passed — graphics needs `s.reverseBg` (top-level
mutation) plus four other slices and stays on `s`, but the
per-slice access path keeps the intent visible.
- **upsertVariable**: rename from `bootstrapVariable` to surface
the silent-backfill semantics (the function does both lookup-
update of an empty `defaultValue` and create). Docstring spells
out the upsert contract.
- **reverseBg docstring**: acknowledge the parallel with
`comment.fnNumber` ("nullable pending until next field consumes
it" — same shape, different home). Document why it's still flat:
one such field at this level, grouping it solo would add
ceremony.
Plus discovered (NOT fixed in this commit, ticketed as
`project_ticket_sn_lifecycle_leak`): `s.field.snPending` /
`snIncrement` / `snMode` are grouped under `field.*` but their
reset semantics differ from the rest — only the text-branch of
flushField clears them. Non-text field after ^SF leaks. Pre-
existing, surfaced by the slice grouping.
1409 tests pass; lint 0 errors; build green.
…ping
Pragmatic schulds the prior refactors stepped on but didn't fix.
Seven idiomatic collapses in one pass — every one of them was
already a pattern duplicated across 2-12 sites in the parser
subtree.
**Shared helpers (context.ts):**
- `pushBrowserLimit(result, token)` — replaces the
`result.skipped.push(X); result.browserLimit.push(X);` paired
push (6 sites in graphics.ts, 5 inline in unsupported.ts).
Encodes the `browserLimit ⊆ skipped` subset invariant once.
- `getDefaultTextH(defaults)` / `getDefaultTextW(defaults)` —
replaces 7 inline `s.defaults.cfHeight || 30` / `cfWidth || 0`
fallback chains across fields.ts + parseZPL.ts dynamic ^A. The
"30 / 0 are the historical baselines" invariant now lives in
one place.
- `getPosType(field)` — collapses 3 sites of
`const posType: "FT" | "FO" = field.positionIsFT ? "FT" : "FO"`
in flushField + graphics. The literal-pair can't drift.
**Shared helpers (helpers.ts):**
- `firstChar(rest)` — replaces 4 sites of
`(rest.trim()[0] ?? "").toUpperCase()` across labelConfig single-
char enum handlers. The "lenient about leading whitespace,
case-insensitive" idiom is now one place.
- `readColor(raw)` — replaces 4 `(p[N] ?? "B") as "B" | "W"`
unchecked type assertions in graphics.ts (^GB / ^GC / ^GD / ^GE
colour slot). Sibling of `readRotation` with the same
validate-or-default contract.
**API-surface cleanup (handlers/unsupported.ts):**
- `createUnsupportedHandlers(result: ParserResult)` — was
`({ skipped, browserLimit })`; now takes the whole result slice
so the family uses `pushBrowserLimit(result, …)` consistently
with graphics.
**Documentation (parseZPL.ts):**
- 50-line module-header JSDoc on parseZPL with a pipeline
diagram (tokenize → dispatch → per-family handlers → flushField
→ findings) + state-layout summary + file map. Orients the
next contributor without forcing them to reverse-engineer the
decomposition from imports.
No behavior change. 1409 tests pass; lint 0 errors; build green.
The audit by an external "idiomatic completeness" check flagged
six items pre-push, all addressed here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the monolithic ZPL parser in src/lib/zplParser.ts by modularizing its logic into separate files for context, field flushing, and command handlers (barcodes, fields, graphics, etc.). A review comment identifies a potential TypeScript compilation error in flushField.ts where a string | null property is passed to a function expecting a string, and suggests destructuring the state properties into local variables to ensure robust type narrowing.
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
1 participant
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.
No description provided.