TML-2714: generate one import per module in contract.d.ts by reusing the shared renderer#614
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (22)
📝 WalkthroughWalkthroughThis PR enhances the TypeScript import rendering system to support per-symbol aliasing and type-only import statements, refactors the aggregation model to track binding metadata, integrates the new pipeline into the emitter, and regenerates contract files across the codebase. ChangesImport Aliasing and Type-Only Rendering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/extension-cipherstash
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/1-core/ts-render/src/render-imports.ts`:
- Around line 84-90: group.named is currently keyed only by req.symbol which
collapses distinct local aliases (alias) for the same exported symbol; update
the data structure and logic so bindings are preserved per local alias (e.g.,
change group.named to map symbol -> Map<alias, {typeOnly}> or make the top-level
key a composite of symbol+alias) or alternatively throw when an alias mismatch
is detected; update the insertion logic that uses existing, alias, req.symbol
and typeOnly to either set separate entries per alias or to detect and throw on
conflicting aliases, and add regression tests covering (1) same symbol imported
with two different aliases and (2) symbol imported both plain and as an alias to
ensure behavior is correct and order-independent.
- Around line 136-167: The code currently generates an invalid single "import
type" statement when isStatementTypeOnly(group) is true but the group has both a
defaultSymbol and named bindings; update renderModuleImport (and stop relying on
buildImportClause to produce the combined form in that case) so that when
statementTypeOnly is true and both defaultSymbol and named.size>0 you emit two
separate imports: one "import type <Default> from 'module'<attrs>;" and one
"import type { <Named...> } from 'module';" (or alternatively reject/throw on
that input); adjust buildImportClause to avoid producing "Default, { Named }"
for type-only statements and add a regression test covering default+named
type-only imports to ensure the split behavior (reference: renderModuleImport,
buildImportClause, isStatementTypeOnly, renderNamedBinding).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b6f6e847-d06b-4684-900b-d05f4c03068b
⛔ Files ignored due to path filters (6)
packages/3-extensions/sql-orm-client/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltest/e2e/framework/test/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/mongo/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/sql-builder/fixtures/generated/contract.d.tsis excluded by!**/generated/**test/integration/test/sql-orm-client/fixtures/generated/contract.d.tsis excluded by!**/generated/**
📒 Files selected for processing (22)
apps/telemetry-backend/src/prisma/contract.d.tsexamples/cipherstash-integration/src/prisma/contract.d.tsexamples/mongo-blog-leaderboard/src/contract.d.tsexamples/mongo-demo/src/contract.d.tsexamples/paradedb-demo/src/prisma/contract.d.tsexamples/prisma-next-cloudflare-worker/src/prisma/contract.d.tsexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-postgis-demo/src/prisma/contract.d.tsexamples/react-router-demo/src/prisma/contract.d.tsexamples/retail-store/src/contract.d.tspackages/1-framework/1-core/ts-render/src/render-imports.tspackages/1-framework/1-core/ts-render/src/ts-expression.tspackages/1-framework/1-core/ts-render/test/render-imports.test.tspackages/1-framework/3-tooling/emitter/package.jsonpackages/1-framework/3-tooling/emitter/src/domain-type-generation.tspackages/1-framework/3-tooling/emitter/test/domain-type-generation.test.tspackages/3-extensions/cipherstash/src/contract.d.tspackages/3-extensions/paradedb/src/contract.d.tspackages/3-extensions/pgvector/src/contract.d.tspackages/3-extensions/postgis/src/contract.d.tspackages/3-extensions/sql-orm-client/scripts/strip-pgvector-fixture.mjstest/integration/test/fixtures/contract.d.ts
Generated contract.d.ts files emitted duplicate import type lines because the emitter generateImportLines produced one line per import spec with no per-module aggregation. Route it through @prisma-next/ts-render renderImports (extended with alias and type-only support) so imports are aggregated per module specifier, matching the migration renderers. Regenerate affected fixtures. Refs: TML-2714 Signed-off-by: Will Madden <madden@prisma.io>
… default+named imports
Bug 1: ModuleImportGroup.named was keyed by symbol alone, so { A } + { A as B } and { A as B } + { A as C } collapsed into a single binding. Key the named-bindings map by (symbol, alias) instead, store the symbol on the binding, and sort by (symbol, alias) using code-unit comparison so the un-aliased form precedes any aliased form of the same symbol. Identical (symbol, alias) pairs still merge typeOnly by AND.
Bug 2: When isStatementTypeOnly was true and the group had both a default and named bindings, buildImportClause emitted import type Default, { Named } which TS rejects (TS1363). Split that case into two valid type-only statements (import type Default from "m"; then import type { Named } from "m";). The non-type-only mixed case still renders on a single line.
JSDoc updated to document the type-only default+named exception, the (symbol, alias) sort key, and the distinct-bindings semantics.
Refs: TML-2714
Signed-off-by: Will Madden <madden@prisma.io>
7a44338 to
ad36c45
Compare
…istory Replace the three synthetic normal-shape golden cases with cases drawn from real merged PRs, so the corpus measures Drive runs against work the team actually shipped rather than synthesised tasks: - direct-change-example-emit-outputpath (TML-2722 / #618) - slice-dedupe-generated-imports (TML-2714 / #614) - project-reap-subsumed-ir-surfaces (TML-2727 / #630, #631, #629) — a three-slice parallel fan-out that exercises planner parallelisation and scope discipline. Each real case carries the task as posed (Linear ticket, solution-scrubbed so the run still does the design/planning), a base_sha to run against, and a reference.md describing the known-good output by commit SHA (the output itself is fetchable via git diff <base_sha> <merge_sha>). case.json gains source + base_sha; the loader ignores the extra fields until the experiment-engine slice wires base_sha into a checkout. The two pathological cases (i12-halt, spike-first) stay synthetic: no clean merged PR exhibits a halted or spiked run. Update harness tests, SKILL.md examples, and the corpus README for the renamed slugs. validate-parser fixtures are left as-is — they are synthetic parser fixtures with tuned event counts, not corpus members. Signed-off-by: wmadden-electric <286902546+wmadden-electric@users.noreply.github.com>
At a glance
Generated
contract.d.tsfiles repeated the same module across separateimport typelines. This showed up in every Mongo contract, for instance:The decision
Route the emitter's import generation through
renderImports— the import aggregator in@prisma-next/ts-renderthat the migration renderers already use — instead of keeping the emitter's own separate renderer. Teach that shared aggregator the two things the emitter needed but it didn't yet support (per-symbol aliases andimport type), so one canonical renderer serves both call sites.How we got here
The duplication was reported against Mongo output, but it was family-agnostic — the same repeated-import pattern appeared across SQL and Document targets, anywhere a module contributed more than one imported type.
The cause was two independent import renderers:
renderImportsin@prisma-next/ts-render, which aggregates all named imports for a module onto one statement. Correct.generateImportLines, which emitted one line per import spec with no per-module aggregation. That is what produced the duplicates.Two renderers solving the same problem had already drifted once — that drift is this bug. The fix collapses them to one.
The change
generateImportLinesnow maps the emitter'sTypesImportSpec[]toImportRequirement[]and delegates torenderImports.renderImportsgains the only capabilities the emitter required that it lacked: per-symbol aliases (CodecTypes as MongoCodecTypes) and type-only imports (import type), with stable sort order. Both are visible in the at-a-glance example — the merged line keeps the alias and theimport typemodifier.Verification
pnpm buildpnpm fixtures:checkpnpm --filter @prisma-next/ts-render test(32) ·pnpm --filter @prisma-next/emitter test(153)pnpm lint:deps·pnpm typecheckOut of scope
paradedb-demoandprisma-next-demo-sqliteemit generated output toprisma/while their tracked artifacts live insrc/prisma/— a pre-existing outDir mismatch unrelated to import dedup. Left for a follow-up.Alternatives considered
generateImportLinesin place. Rejected: it leaves two renderers solving the same problem. They drifted once already; fixing only the emitter invites the next divergence.@prisma/ts-builders). Deferred. That is a much larger change spanning declarations, type aliases, and doc comments across the emitter and the migration renderers, tracked separately in TML-2272. This PR takes the minimal consolidation that fixes the reported bug now and proves the pattern.Refs: TML-2714.