-
-
Couldn't load subscription status.
- Fork 126
fix: several compilation issues with the new "prisma-client" generator #2270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughEnhancer now detects which Prisma client generator is used to choose "client" vs "models", limits enhancement generation for the new client to the "node" target, and conditionally emits and re-exports Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Build as Schema build
participant Enhancer as Enhancer plugin
participant GenDetect as getPrismaClientGenerator
participant FS as File writer
Build->>Enhancer: run enhance(model, options)
Enhancer->>GenDetect: getPrismaClientGenerator(model)
GenDetect-->>Enhancer: { isNewPrismaClientGenerator }
alt new prisma-client
Enhancer->>Enhancer: prismaClientPath := ".../client"
Enhancer->>Enhancer: targets := ["node"]
else legacy prisma-client
Enhancer->>Enhancer: prismaClientPath := ".../models"
Enhancer->>Enhancer: targets := ["node","edge"]
end
Enhancer->>Enhancer: hasTypeDefs? = any model is a type
alt has type defs
Enhancer->>FS: write `json-types.ts`
Enhancer->>FS: write `models.ts` exporting base models + `./json-types`
else no type defs
Enhancer->>FS: write `models.ts` exporting base models only
end
loop per target in targets
Enhancer->>FS: generateEnhance(target, prismaClientPath)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/schema/src/plugins/enhancer/enhance/index.ts(3 hunks)packages/schema/src/plugins/enhancer/index.ts(2 hunks)packages/sdk/src/code-gen.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/sdk/src/code-gen.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/schema/src/plugins/enhancer/enhance/index.ts (2)
packages/language/src/generated/ast.ts (1)
isTypeDef(650-652)packages/sdk/src/code-gen.ts (1)
saveSourceFile(25-37)
packages/schema/src/plugins/enhancer/index.ts (1)
packages/sdk/src/utils.ts (1)
getPrismaClientGenerator(700-723)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
packages/schema/src/plugins/enhancer/enhance/index.ts (3)
132-145: LGTM!The conditional export of
json-typesis correct. It only exports when type definitions exist in the model, which prevents importing a non-existent file.
606-612: LGTM!The conditional generation of
json-types.tsis correct. The file is only created when type definitions exist, which prevents empty file generation and aligns with the conditional export inmodels.ts.
168-171: Verify removal of the ‘edge’ target for the new Prisma Client generator
No documentation (e.g. CHANGELOG, migration guide) or tests reference edge support; if this is a breaking change, document it and update or remove any edge-related tests.packages/schema/src/plugins/enhancer/index.ts (1)
39-40: Confirm fallback to 'models' when Prisma client generator is missing
The use of optional chaining means that ifgetPrismaClientGeneratorreturnsundefined, the path will default to'models'. Ensure this fallback is intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/tanstack-query/tests/plugin.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: OSSAR-Scan
- GitHub Check: build-test (20.x)
- GitHub Check: dependency-review
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
🔇 Additional comments (1)
packages/plugins/tanstack-query/tests/plugin.test.ts (1)
347-347: LGTM! Simplified dependency declaration.Removing the npm alias indirection (
@tanstack/angular-query-v5@npm:...) in favor of directly referencing@tanstack/angular-query-experimental@5.84.xmakes the dependency clearer and easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/server/tests/adapter/hono.test.ts (1)
85-87: Same issue as in sveltekit.test.ts—investigate systematically.This is the same test skip pattern as in
packages/server/tests/adapter/sveltekit.test.ts:84-86. Since both Hono and SvelteKit adapters are affected, this suggests a systemic issue rather than an adapter-specific problem. The investigation should address both test files together.Verify whether other adapter test files have similar skipped tests:
#!/bin/bash # Search for other skipped custom load path tests echo "=== Finding all it.skip patterns in adapter tests ===" fd -e ts -e js . packages/server/tests/adapter/ --exec rg -n "it\.skip.*custom.*load" {} \; echo "" echo "=== Checking for TODO comments in adapter tests ===" fd -e ts -e js . packages/server/tests/adapter/ --exec rg -n "TODO.*CI|TODO.*investigate" {} \;
🧹 Nitpick comments (1)
packages/sdk/src/typescript-expression-transformer.ts (1)
513-532: Clarify the comment about compile-time evaluation.The compile-time evaluation logic is sound and correctly identifies expressions that resolve to compile-time constants (literals,
currentModel(),currentOperation()). By comparing the transformed string representations, it accurately determines if the comparison will always yield the same result and short-circuits to a boolean literal.However, the comment on line 518 says "to avoid TS compiler errors," which is slightly misleading. The expressions like
('User' == 'User')are valid TypeScript; this optimization is more about:
- Producing cleaner generated code
- Avoiding constant-condition warnings (if strict linting is enabled)
- Runtime performance (eliminating redundant comparisons)
Consider clarifying the comment:
- // resolve trivial comparisons to avoid TS compiler errors + // resolve constant comparisons at compile time for cleaner generated codeAdditionally, consider adding test coverage for this optimization to ensure it correctly handles:
- Literal-to-literal comparisons:
'foo' == 'foo'→'true'- currentModel comparisons:
currentModel() == 'User'→'true'(in User model)- currentOperation comparisons:
currentOperation() != 'read'→ depends on context- Mixed cases:
currentModel('upper') == 'USER'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/schema/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (6)
packages/sdk/src/typescript-expression-transformer.ts(2 hunks)packages/server/tests/adapter/hono.test.ts(1 hunks)packages/server/tests/adapter/sveltekit.test.ts(1 hunks)tests/integration/tests/cli/generate.test.ts(1 hunks)tests/integration/tests/cli/plugins.test.ts(2 hunks)tests/integration/tests/enhancements/with-delegate/plugin-interaction.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/sdk/src/typescript-expression-transformer.ts (1)
packages/language/src/generated/ast.ts (2)
isLiteralExpr(69-71)isInvocationExpr(486-488)
tests/integration/tests/cli/generate.test.ts (1)
packages/testtools/src/schema.ts (1)
installPackage(68-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: build-test (20.x)
- GitHub Check: OSSAR-Scan
- GitHub Check: dependency-review
🔇 Additional comments (6)
tests/integration/tests/cli/plugins.test.ts (2)
89-89: LGTM!Adding
@types/node@20is appropriate for ensuring type compatibility with Node.js LTS version 20.
77-77: Ignore tRPC v10 pinning for CLI plugin tests. CLI integration tests are explicitly targeting tRPC v10; v11 compatibility is covered in dedicated plugin tests.Likely an incorrect or invalid review comment.
tests/integration/tests/cli/generate.test.ts (1)
48-48: LGTM!The addition of
@types/node@20is consistent with similar changes across the test suite and ensures proper TypeScript type definitions for Node.js 20 APIs.tests/integration/tests/enhancements/with-delegate/plugin-interaction.test.ts (1)
55-55: Verify tRPC v10 pinning across all packages.All three tRPC packages are now pinned to major version 10 (
@trpc/client@10,@trpc/server@10,@trpc/react-query@10). While this ensures version consistency across the tRPC ecosystem, verify that v10 is intentionally used instead of the current stable v11 release.Based on learnings.
This verification can use the same script suggested for
tests/integration/tests/cli/plugins.test.tsat line 77.packages/server/tests/adapter/sveltekit.test.ts (1)
84-86: Investigate and track CI failure for SvelteKit custom load path test
Confirm whether the new generator structure brokeloadSchema({ output: './zen' })for SvelteKit (and Hono), update the implementation if needed, or open a GitHub issue and reference it in the TODO.packages/sdk/src/typescript-expression-transformer.ts (1)
19-19: LGTM!The import is necessary for the new compile-time evaluation logic added below.
fixes #2243