Skip to content

fix(data,export): collapse the STEP string escaper's two TS copies into one (#3300) - #3378

Open
BIMvoice wants to merge 2 commits into
mainfrom
fix-3300-step-escaper-collapse
Open

fix(data,export): collapse the STEP string escaper's two TS copies into one (#3300)#3378
BIMvoice wants to merge 2 commits into
mainfrom
fix-3300-step-escaper-collapse

Conversation

@BIMvoice

Copy link
Copy Markdown
Collaborator

Closes part of #3300.

The three sites, before this change

packages/data/src/step-serializers.ts escapeStepString, module-private
packages/export/src/step-serialization.ts escapeStepString, exported
rust/export/src/step_text.rs escape (pub(crate))

The two TypeScript bodies were byte-identical: same three-transform pipeline (backslash doubling, quote doubling, one-space-per-control-char), same \X2\/\X4\ non-ASCII directive thresholds, same 32–126 basic-graphic-range check. packages/export/package.json already declares "@ifc-lite/data": "workspace:^" — one direction, no cycle — so there was no dependency reason for the second copy.

Verified today's three implementations actually agree

Before collapsing anything, I ran the same awkward inputs — a bare backslash, an apostrophe, a BMP non-ASCII character, an already-\X2\-looking sequence, control-char runs, and the empty string — through all three (TS data, TS export, and a temporary Rust unit test calling step_text::escape directly). All three agreed on every case. No live divergence bug; this was pure duplication.

What changed

  • @ifc-lite/data's step-serializers.ts now exports escapeStepString (was module-private) and is re-exported from the package's index.ts.
  • @ifc-lite/export's step-serialization.ts deletes its own escapeStepString/encodeNonAsciiStepDirectives body and instead does import { escapeStepString } from '@ifc-lite/data' + export { escapeStepString }, so all five existing call sites (property-value-serialization.ts, step-georeferencing.ts, retype.ts, step-property-set-generators.ts, attribute-slot-types.ts) keep importing from ./step-serialization.js unchanged.
  • scripts/api-surface.json gains the one new @ifc-lite/data export entry (checked with check-api-surface.mjs, matches exactly).

The Rust implementation is left alone: sharing it with TypeScript would need a wasm adapter, which is out of scope here, so the TS/Rust agreement stays pinned by a hand-kept vector test rather than shared code — same reasoning #3297 gave for the sibling lexical-rule issue (#3303).

#3284's parity test

A prior fix (#3284) added a RUST_VECTORS table + it.each block in both TS test files, asserting each copy's control-character-run output matched Rust's observed output. With one TS implementation left:

  • packages/export/src/step-serialization.test.ts's two describe blocks (non-ASCII directive tests + control-char-run parity table) are replaced by a single small "re-export resolves to a working function" smoke test — coverage of the actual behavior would otherwise be a second copy of the same assertions against the same underlying function.
  • packages/data/src/step-serializers.test.ts keeps the full RUST_VECTORS parity suite (comment updated to say it's the one remaining TS copy) and gains new direct tests of escapeStepString (backslash, apostrophe, non-ASCII, already-escaped-looking text, empty string) now that the function is exported instead of reached only through serializeValue/generateHeader.

Mutation-checked: reverting the backslash-doubling regex to a no-op reds 3 tests in data's suite and 2 in export's re-export smoke test, restored cleanly by SHA.

The fourth, partial copy — left alone

packages/encoding/src/ifc-string.ts's encodeIfcString is a genuinely different implementation, not a drop-in: it encodes the whole Latin-1 range as \X\HH (not \X2\HHHH\X0\), routes a backslash into that same arm as \X\5C instead of doubling it, and does no '' quote-doubling or control-to-space mapping. It has no production call sites (only its own ifc-string.parity.test.ts). Folding it in is explicitly out of scope per the issue.

Verification

CARGO_TARGET_DIR=/tmp/t-3300 cargo test -p ifc-lite-export   # 356+ passed, 0 failed (unchanged)
pnpm exec turbo build                                          # full workspace build, clean
pnpm exec turbo typecheck --filter=@ifc-lite/data --filter=@ifc-lite/export --filter=@ifc-lite/codegen
pnpm exec turbo test --filter=@ifc-lite/data --filter=@ifc-lite/export --filter=@ifc-lite/codegen
  @ifc-lite/data:    Test Files 20 passed | Tests 231 passed
  @ifc-lite/export:  Test Files 78 passed, 2 skipped | Tests 1023 passed, 30 skipped
  @ifc-lite/codegen: Test Files 9 passed | Tests 122 passed
node scripts/check-api-surface.mjs   # ✅ matches snapshot
node scripts/check-changesets.mjs    # 4 pending, all naming workspace packages
pnpm exec oxlint <changed files>     # clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

…to one (#3300)

@ifc-lite/data's step-serializers.ts and @ifc-lite/export's
step-serialization.ts each kept a byte-identical escapeStepString
(backslash/quote doubling, \X2\/\X4\ non-ASCII directives, one space
per control char). export already depends on data with no cycle, so
data now exports the function and export re-exports it instead of
keeping its own body; every existing call site is unaffected.

The rust implementation (ifc_lite_export::step_text::escape) stays
separate -- a wasm adapter to share it is out of scope here -- and
stays pinned by a hand-kept vector test rather than shared code.

#3284's parity test, which asserted the two TS copies agreed with
Rust, is gone from the export package (there is nothing left to
duplicate there) and the coverage lives once in data's
step-serializers.test.ts, which also gains direct tests of
escapeStepString now that it is exported.

Verified today's three implementations agree on backslash, apostrophe,
non-ASCII, an already-escaped-looking sequence, and the empty string
before collapsing. The encoding package's separate encodeIfcString is
a genuinely different, partial implementation (whole-Latin-1 \X\HH
range, no '' doubling) and is left alone per the issue.

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
@BIMvoice
BIMvoice requested a review from louistrue as a code owner August 28, 2026 06:32
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 7 billable files and costs up to $1.75.

Or wait 29 minutes for your next included review.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4efa100f-2efa-487b-b529-a959efacfaae

📥 Commits

Reviewing files that changed from the base of the PR and between 5a431e5 and fc70ee9.

📒 Files selected for processing (7)
  • .changeset/collapse-step-escaper-copies.md
  • packages/data/src/index.ts
  • packages/data/src/step-serializers.test.ts
  • packages/data/src/step-serializers.ts
  • packages/export/src/step-serialization.test.ts
  • packages/export/src/step-serialization.ts
  • scripts/api-surface.json

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Viewer benchmark

✅ No threshold regressions detected.

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 1472ms 2905ms -49.3% +50%
firstVisibleGeometryMs 1914ms 3652ms -47.6% +50%
streamCompleteMs 2286ms 3598ms -36.5% +50%
spatialReadyMs 1067ms 1032ms +3.4% +50%
metadataCompleteMs 1562ms 3063ms -49.0% +50%
totalWallClockMs 2400ms 3700ms -35.1% +50%

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 279ms 1075ms -74.0% +50%
firstVisibleGeometryMs 1144ms 1572ms -27.2% +50%
streamCompleteMs 1609ms 1980ms -18.7% +50%
spatialReadyMs 799ms 915ms -12.7% +50%
metadataCompleteMs 1173ms 1392ms -15.7% +50%
totalWallClockMs 2800ms 3300ms -15.2% +50%

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

…dget

`step-serializers.ts` sits at a 459-line budget and the #3300 note pushed it
to 465, failing the 'Check TypeScript module size ratchet' step in Node tests.

Trimmed the added comment rather than raising the budget: a reviewer flagged a
budget raise on a sibling PR today, and the house rule treats a raise as a last
resort needing written justification. The note still says what matters -- this
is the one TS implementation, export re-exports it, and a vector test pins
parity with the separate Rust escaper.
@vercel

vercel Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
ifc-lite-dev Ignored Ignored Preview Aug 28, 2026 6:49am
ifc-lite-viewer-embed Ignored Ignored Aug 28, 2026 6:49am

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.

1 participant