-
-
Notifications
You must be signed in to change notification settings - Fork 127
Make enhance function compatible with prisma v7 #2310
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
📝 WalkthroughWalkthroughSelects Prisma runtime import subpath based on detected Prisma version, imports Changes
Sequence Diagram(s)sequenceDiagram
participant Enhancer
participant VersionChecker
participant Generator
Enhancer->>VersionChecker: getPrismaVersion()
VersionChecker-->>Enhancer: version (e.g., 6.8.0 or 7.1.0)
alt version >= 7.0.0
note right of Enhancer `#E6F0FF`: New-client path
Enhancer->>Enhancer: runtimeSubPath = '/runtime/client'
Enhancer->>Generator: generateLogicalPrisma(import='@prisma/client/runtime/client', flags=[...])
else version < 7.0.0
note right of Enhancer `#FFF5E6`: Legacy-client path
Enhancer->>Enhancer: runtimeSubPath = '/runtime/library'
alt version >= 5.2.0
Enhancer->>Generator: generateLogicalPrisma(import='@prisma/client/runtime/library', flags=['--no-engine', ...])
else
Enhancer->>Generator: generateLogicalPrisma(import='@prisma/client/runtime/library', flags=[...])
end
end
Generator-->>Enhancer: generated artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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
🧹 Nitpick comments (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
437-442: Align--no-enginecondition with “version supports” comment and avoid unknown-version riskThe condition:
if (!prismaVersion || (semver.gte(prismaVersion, '5.2.0') && semver.lt(prismaVersion, '7.0.0'))) {adds
--no-engineeven whenprismaVersionis unknown, but the comment says “if the prisma version supports”. For an unknown or older Prisma, this can causeprisma generateto fail with an unrecognized flag, and the retry path reuses the same command.To better match the comment and be safer for unknown versions:
- const prismaVersion = getPrismaVersion(); - if (!prismaVersion || (semver.gte(prismaVersion, '5.2.0') && semver.lt(prismaVersion, '7.0.0'))) { + const prismaVersion = getPrismaVersion(); + if (prismaVersion && semver.gte(prismaVersion, '5.2.0') && semver.lt(prismaVersion, '7.0.0')) { // add --no-engine to reduce generation size if the prisma version supports // v7 has removed this option completely, because it no longer generates an engine generateCmd += ' --no-engine'; }That way
--no-engineis only used when you positively know the version is in the supported range.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/schema/src/plugins/enhancer/enhance/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
packages/sdk/src/prisma.ts (1)
getPrismaVersion(60-79)
ymc9
left a 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.
Hi @jamiter , thanks for making this PR. Could you take a look at my comments?
v7 has the new /runtime/client path.
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
♻️ Duplicate comments (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
318-320: Critical: Guardsemver.gteagainst undefined Prisma versionThis issue was already identified in a previous review and remains unfixed.
getPrismaVersion()returnsstring | undefined, so callingsemver.gte(prismaVersion, '7.0.0')without a guard will cause type-checking failures and runtime errors when the version cannot be determined. Othersemver.gtecalls in this file (lines 341, 438) are properly guarded.Apply this diff to add the guard:
const prismaVersion = getPrismaVersion(); -const runtimeLibraryImportSubPath = semver.gte(prismaVersion, '7.0.0') ? '/runtime/client' : '/runtime/library'; +const runtimeLibraryImportSubPath = + prismaVersion && semver.gte(prismaVersion, '7.0.0') ? '/runtime/client' : '/runtime/library';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/schema/src/plugins/enhancer/enhance/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
packages/sdk/src/prisma.ts (1)
getPrismaVersion(60-79)
🔇 Additional comments (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
437-442: Verify intended behavior for undefined Prisma versionThe condition
!prismaVersion || (semver.gte(prismaVersion, '5.2.0') && semver.lt(prismaVersion, '7.0.0'))applies the--no-engineflag when the version is undefined OR when it's in the range [5.2.0, 7.0.0). However, the AI-generated summary states the flag should be applied "only when Prisma version exists and is in [5.2.0, 7.0.0)", which would exclude the undefined case.Please confirm the intended behavior:
- Should
--no-enginebe applied when the version cannot be determined (current behavior)?- Or should it only be applied when the version is known to be in [5.2.0, 7.0.0) (as per AI summary)?
If the latter is correct, the condition should be:
-if (!prismaVersion || (semver.gte(prismaVersion, '5.2.0') && semver.lt(prismaVersion, '7.0.0'))) { +if (prismaVersion && semver.gte(prismaVersion, '5.2.0') && semver.lt(prismaVersion, '7.0.0')) {
And implement other AI feedback about checking `prismaVersion`
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 (1)
packages/schema/src/plugins/enhancer/enhance/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
packages/sdk/src/prisma.ts (1)
getPrismaVersion(60-79)
🔇 Additional comments (1)
packages/schema/src/plugins/enhancer/enhance/index.ts (1)
317-334: Prisma v7 runtime path handling looks correctUsing
runtimeLibraryImportSubPathto switch between/runtime/clientfor Prisma>= 7.0.0and/runtime/libraryotherwise, plus the guardedgetPrismaVersion()/semver.gtecheck, aligns with the reported v7 layout and avoids callingsemveronundefined. The template literal forruntimeLibraryImportalso fixes the earlier non-interpolated string issue.No further changes needed here.
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.
LGTM! Thank you @jamiter ! Will merge when CI passes and include it in the upcoming release.
This was for us the only thing required (together with #2305) to make zenstack compatible with prisma v7.
Related: #2309
@ymc9, would you consider these changes?