feat(ts): emit per-proto type modules + shared errors in TS generators

[hishamank]

What

Both TypeScript generators (protoc-gen-ts-client, protoc-gen-ts-server) now emit a canonical per-proto type module layout:

• One <proto>.ts module per source proto that defines messages/enums (e.g. core/v1/identifiers.ts), declaring each type once.
• A shared errors.ts at the output root holding the runtime helpers (ApiError, ValidationError, FieldViolation).
• Slimmed _client.ts / _server.ts service files containing only the service class / handlers + framework types, importing their request/response types from the type modules and helpers from errors.ts via relative specifiers.

Why

Previously each service file inlined the full transitive type closure, so shared types were re-declared in every file (SongID ×5, ValidationError ×14 in a real consumer). Runtime helpers were likewise duplicated per file. The new layout declares each type once and imports it — matching how ts-proto / protobuf-es structure their output — which is what makes the generated client usable as a real npm types package.

Behavior

• This is the default and only layout — there is no configuration flag.
• Requires strategy: all in buf.gen.yaml so the shared modules (errors.ts and cross-package type modules) are emitted once over the whole file graph instead of once per directory.

Notes

• Breaking change for existing consumers: the shape of the emitted files changes (types move out of _client.ts/_server.ts into <proto>.ts + errors.ts, and imports are added). Import paths in downstream code must be updated accordingly.
• A proto message literally named ValidationError, ApiError, or FieldViolation now errors loudly (it would shadow the hoisted error helpers).
• Client vs server type modules are byte-identical (they share the tscommon emitter).

Tests

• Golden harnesses for both generators rewritten to walk the full generated output tree and compare every emitted .ts file; all fixtures regenerated to the modules layout.
• httpgen cross-generator consistency tests updated to read the type module + service file together.
• Server validation errors (path-param / field-coverage) preserved and still enforced.
• Full suite + staticcheck green.

[github-actions]

🔍 CI Pipeline Status

✅ Lint: success
✅ Test: success
✅ Coverage: success
✅ Build: success
✅ Integration: success

---

📊 Coverage Report: Available in checks above
🔗 Artifacts: Test results and coverage reports uploaded

[codecov]

Codecov Report

❌ Patch coverage is 47.63158% with 199 lines in your changes missing coverage. Please review.
✅ Project coverage is 10.99%. Comparing base (b3f6487) to head (d5531b9).

Files with missing lines	Patch %	Lines
internal/tscommon/modules.go	0.00%	111 Missing ⚠️
internal/tscommon/types.go	0.00%	62 Missing ⚠️
internal/tscommon/imports.go	78.26%	23 Missing and 2 partials ⚠️
internal/tsservergen/generator.go	95.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##            main     #200      +/-   ##
=========================================
+ Coverage   3.32%   10.99%   +7.66%     
=========================================
  Files         62       66       +4     
  Lines      10966    11219     +253     
=========================================
+ Hits         365     1233     +868     
+ Misses     10595     9967     -628     
- Partials       6       19      +13     

Flag	Coverage Δ
unittests	10.99% <47.63%> (+7.66%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SebastienMelki]

Final Review — PR #200: feat(ts): emit per-proto type modules + shared errors in TS generators

Branch: feat/ts-import-modules → main · Author: Hicham (@hishamank) + Claude
Size: 114 files, +4,889 / −2,467 (mostly regenerated golden fixtures)
Basis: Consolidation of two independent reviews (claude-review.md, codex-review.md).
Every finding below was reproduced or verified directly against a freshly built plugin on
this branch.

---

Verdict

Request changes before merge. The refactor itself is well-architected and valuable — one
canonical type module per proto, a shared errors.ts, per-package barrels, and
nodenext-correct .js imports, with a clean design (single chokepoint for relative-import
formatting, single source of truth for TS type names, byte-identical client/server type
modules). But the two reviews together surface one High and three Medium correctness/UX
defects that produce invalid or scattered output for realistic inputs. None is hard to fix.

The two reviews were highly complementary: Codex found the two real generation bugs (enum
collision, path-basis divergence); the Claude pass found the ecosystem/packaging gaps (examples
not migrated, barrel collision) and the testing-harness caveat. There were no contradictions
between them.

---

Findings (ranked)

🔴 High 1 — Enum names are not checked against reserved error-helper names → invalid TypeScript

Source: codex-review.md #1. Independently reproduced.

CheckReservedNames (internal/tscommon/modules.go:60) only walks ms.OrderedMessages().
Enums are collected and emitted separately (GenerateEnumType, types.go:414) and are still
reachable from service modules via root-unwrap, path-param enum casts, and query enum parsing —
all of which call ctx.RefEnum. So an enum named ApiError / ValidationError /
FieldViolation sails past the guard and is imported alongside the runtime helper of the same
name.

Reproduced with an enum named ApiError (source_relative). Generated collide_client.ts:

import { ApiError, ValidationError } from "./errors.js";
import type { ApiError, GetRequest } from "./collide.js";   // duplicate identifier — TS2300/TS2440

The server module has the same shape. This is invalid TypeScript.

Related (Claude Low 1): even for messages, CheckReservedNames compares the leaf
Desc.Name() rather than the emitted QualifiedTSName, giving both a false positive (nested
Wrapper.ValidationError → WrapperValidationError, needlessly rejected) and a false negative
(top-level Validation with nested Error → emits ValidationError, not caught).

Recommended fix (covers both): make the reservation robust in ImportTracker — reserve the
three runtime-helper names up front so any imported type/enum whose local name collides is
deterministically aliased (ApiError_1), the mechanism the tracker already has. Optionally
also extend CheckReservedNames to iterate enums and compare QualifiedTSName, if you prefer
a loud rename error over silent aliasing. Reserving in the tracker is the more complete fix
because it handles enums, qualified names, and future helpers uniformly.

🟡 Medium 1 — Type-module path ignores GeneratedFilenamePrefix; default protoc path mode scatters output and breaks barrels

Source: codex-review.md #2. Independently reproduced.

Service modules are emitted at file.GeneratedFilenamePrefix (tsclientgen/modules.go:33,
tsservergen/modules.go:37), but type modules are emitted at ModuleForFile(srcPath) using the
raw proto path (tscommon/modules.go:165). These agree only under
paths=source_relative. Under the default protoc/protogen path mode
(go_package = ".../v1;collidev1"), they diverge:

collide.ts                                 <- type module (source-relative root)
errors.ts                                  <- shared helpers (root)
example.com/collide/v1/collide_client.ts   <- service module (Go import path)
example.com/collide/v1/index.ts            <- barrel, in the WRONG directory

The cross-module import path itself is computed correctly (../../../collide.js resolves back to
the root type module), so imports don't dangle — but the per-package barrel breaks: it sits
in the service-module directory and re-exports only ./collide_client.js, never the type module
(which is at the root). "Import the whole package by its directory" therefore silently omits every
message/enum type. The layout is also just surprising (types at root, services nested).

Recommended fix: derive the type-module path from the same basis as
GeneratedFilenamePrefix (so both honor the active path mode), or detect an unsupported path
mode and fail loudly with a clear message ("the modules layout requires
paths=source_relative"). A silent, subtly-broken layout is the worst outcome.

🟡 Medium 2 — Repo's own examples are not migrated to the now-required strategy: all

Source: claude-review.md Medium 1.

The PR documents that the modules layout requires strategy: all (otherwise buf runs the
plugin per-directory and the shared/cross-package modules are emitted from multiple invocations —
duplicate-file errors or broken cross-package imports). None of the four TS example configs set
it, and the PR doesn't touch them:

• examples/ts-client-demo/buf.gen.yaml
• examples/ts-fullstack-demo/buf.gen.yaml
• examples/sse-streaming/buf.gen.yaml
• examples/rn-client-demo/buf.gen.yaml

Consequence: regenerating any of the repo's own showcases will not "just work," and any committed
generated output / hand-written consumer TS in them still imports the old single-file shape.

Recommended fix: add strategy: all to the four configs and regenerate (and fix example
consumer imports), or explicitly defer with a tracked follow-up issue. Docs/AGENTS.md were
updated; the examples should match.

🟡 Medium 3 — Per-package barrel index.ts collides when client + server share one output directory

Source: claude-review.md Medium 2.

EmitPackageBarrels writes <dir>/index.ts re-exporting the service module. The client barrel
emits export * from "./service_client.js"; the server barrel emits
export * from "./service_server.js". Type modules and errors.ts are byte-identical between
the two generators (good), but the barrels are not — so pointing both generators at one
output directory (a scenario the PR description frames as supported: "consistent type modules")
is last-writer-wins, silently dropping one side's re-export.

The examples dodge this with distinct out: dirs (./client/generated vs ./server/generated)
and the golden trees are separate, so it's untested.

Recommended fix: document that client and server must use distinct output directories (drop
the "same output directory" framing), or make barrel emission additive/merge-safe (e.g. only
re-export type modules from the shared barrel and leave service aggregation to the consumer).

🟢 Low 1 — export * barrels assume exactly one proto package per directory

Source: claude-review.md Low 2 (acknowledged in a code comment). Two files with different
packages sharing one output dir and a same-named top-level type would collide (TS2308).
Uncommon with standard dir == package layouts; worth a one-line doc note.

🟢 Low 2 — UsedErrorSymbols substring matching can over-import

Source: claude-review.md Low 3. A helper name appearing only in a comment/string would still
be imported, tripping noUnusedLocals/verbatimModuleSyntax in strict consumer tsconfigs. The
three names are mutually non-substring, so no cross-contamination; low risk with current
templates.

⚪ Nit — RelativeImportSpecifier mangles a bare-parent-directory target

Source: claude-review.md nit. a/b/c/x → a/b yields rel == ".." → "...js". Not reachable
today (targets are always concrete file modules); defensive only.

---

Coverage gaps (both reviews agree these would have caught the bugs)

1. No typecheck of generated TypeScript. (codex-review.md) The golden tests byte-compare
   .ts files but never run tsc --noEmit under moduleResolution: nodenext. A tiny fixture
   that compiles the generated output would have caught High 1 (duplicate import) directly
   and would back up the PR's Node-ESM compatibility claim. Highest-value follow-up.
2. No orphaned-golden detection. (codex-review.md) Stale golden files that are no longer
   emitted don't fail the suite; a generated-vs-golden set comparison would harden future
   layout changes.
3. A cross-package fixture under default path mode would have caught Medium 1. Existing
   cross-package coverage only exercises paths=source_relative.

---

Verification notes

• ✅ With freshly built plugin binaries, internal/tscommon, internal/tsclientgen,
  internal/tsservergen, and internal/httpgen all pass; go vet clean. Codex reports the same
  four packages green in a sandboxed run. New coverage (in-process runners, cross-package,
  nested-collision, reserved-name fixtures, barrel assertions) is solid.
• ⚠️ Testing-harness caveat (claude-review.md): the subprocess golden tests reuse a
  prebuilt bin/protoc-gen-* and only rebuild if it is missing (golden_test.go:98–105). A
  stale bin/ made the tests run the old generator and report spurious failures/passes on my
  machine. Codex's run passed precisely because a fresh sandbox (GOPATH under /private/tmp,
  no prebuilt binary) forced a rebuild — the two observations corroborate each other. Since this
  PR leans on golden tests for its correctness story, the harness should always rebuild (or CI
  must make build immediately before go test).
• ℹ️ One unrelated environmental failure I saw: internal/openapiv3
TestExhaustiveGoldenFiles/timestamp_format_service_yaml — the diff is entirely
  google.protobuf.Timestamp doc-comment text, i.e. a protoc/WKT version skew (local
  libprotoc 35.0). openapiv3 is untouched by this PR.

---

What's good (worth preserving)

• Single chokepoint (RelativeImportSpecifier) for the ESM .js fix.
• QualifiedTSName as the single source of truth routed through both declaration and reference
  sites — keeps hoisted nested names byte-identical, which is what export * barrels need.
• Byte-identical client/server type modules via the shared tscommon emitter.
• Deterministic output throughout (sorted specifiers/symbols/barrels), which keeps goldens sane.

---

Recommended path to merge

1. Fix High 1 — reserve the runtime-helper names in ImportTracker (and/or extend
   CheckReservedNames to enums via QualifiedTSName). Blocking.
2. Fix Medium 1 — align the type-module path with GeneratedFilenamePrefix, or reject
   unsupported path modes with a clear error. Blocking.
3. Add a tsc --noEmit golden fixture — cheap, and it locks in both fixes plus the Node-ESM
   claim. Strongly recommended in this PR.
4. Fix Medium 2 & 3 — migrate the four examples to strategy: all + regenerate; decide the
   client/server same-dir barrel story (document distinct dirs is the cheap option). Should be
   in this PR or an immediate tracked follow-up.
5. Low/nit items are optional hardening.

[hishamank]

@SebastienMelki all review points handled in a655891..d5531b9:

• High 1 — reserved names now deterministically aliased via ImportTracker reservation (covers enums + qualified nested names); loud error dropped in favor of aliasing
• Medium 1 — non-source_relative path modes now fail loudly with an actionable error
• Medium 2 — all four example configs migrated to strategy: all, consumers updated to the modules-layout imports
• Medium 3 — distinct client/server out: dirs documented; shared-dir framing dropped
• tsc --noEmit — both golden trees now typecheck under strict nodenext in CI; getting it green surfaced and fixed 3 more generator bugs (proto types shadowing template globals like Response, unused wrapper imports on map-value unwrap, unconverted repeated query params)
• Harness — TS golden tests now always build the plugin fresh into a temp dir (no more stale bin/)