Skip to content

feat: add incremental renderTo output - #130

Open
tecfu wants to merge 6 commits into
masterfrom
feat/render-to-writable
Open

tecfu wants to merge 6 commits into
masterfrom
feat/render-to-writable

Conversation

@tecfu

@tecfu tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner

Add a renderTo({ write }) API that emits the already-rendered table in chunks instead of first concatenating the complete output into one large string. This reduces peak output-buffer memory for large tables while preserving the existing render() API and table height semantics.

@tecfu

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: merge after one test update

The core claims check out — I probed beyond the PR's own test:

config render≡renderTo (string) height parity
marginTop: 2 ✅ 12 = 12
borderStyle: none, compact ✅ 8 = 8
solid + footer ✅ 11 = 11
dashed ✅ 11 = 11
invisible ✅ 11 = 11

The lineCount + 1 arithmetic reproduces the old split(/\r\n|\r|\n/).length on every emit path, including the marginTop prefix and the section-break break arms that emit nothing. 71-case matrix: 0 changed lines (additive API).

One required fix: this fails test/table-config-key.test.ts > keeps a spread of the table free of internals — it asserts the table's method keys are exactly ["render"], and renderTo lands before render in insertion order, giving ["renderTo", "render"]. The test's intent (no internals) is preserved — update the expectation to the two methods (or have it assert "no undefined key / no config value") rather than pinning a fixed key list.

Please also:

  • Add the parity assertion I ran ad-hoc (join(chunks) === render(), height equal) to the committed test; toBeGreaterThan(0) alone would pass with a wildly wrong height.
  • README + the Table interface in types.ts/factory don't mention renderTo; it's a public API addition — a usage note belongs in this PR.

Sincerely qwen3.8-flash-next

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: merge after API/test contract cleanup

The implementation correctly factors emission through stringifyData and preserves existing render output when chunks are joined. The public method is also deliberately non-enumerable, which is a sensible compatibility choice.

Before merge, I would tighten three things:

  1. The committed test checks height > 0, but the important invariant is renderTo height == render height. Pin that equality.
  2. The existing table-key/spread test needs to acknowledge the new public method; currently the added method changes the expected method-key set/order.
  3. renderTo is a public API and should be represented in the exported Table type/interface and documented with a small usage example.

I do not see a rendering correctness blocker in the current implementation.

— GPT-5.6 Luna

tecfu commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Re-review: changes addressed

The streaming implementation preserves rendered output while exposing incremental writes, and renderTo is now part of the public Table type. The regression also checks emitted row content.

CI is still running; no remaining implementation blocker found.

— GPT-5.6 Luna

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