-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(cli): add Driver Adapters and underlying drivers to prisma -v
#24448
base: main
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
…lying driver is in devDependencies
packages/cli/src/__tests__/commands/__snapshots__/Version.test.ts.snap
Outdated
Show resolved
Hide resolved
|
||
const adapterVersionsWithUnderlyingDrivers = adapterVersions.map(([adapterName, adapterVersion]) => { | ||
const underlyingDriver = underlyingDriverAdaptersMap[adapterName] | ||
let underlyingDriverVersion = pkgJson.dependencies?.[underlyingDriver] ?? null |
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.
So, that potentially will give us a version range, not the actual version: (^5.0.0
can really be any version of Prisma 5). Can we try to require (const require = createRequire(pkgJsonPath); require(adapterName).version
the package and fallback onto the range only if that fails?
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.
Wouldn't the behavior you're suggesting be different than what prisma version
currently does?
I agree that having the precise version would be better, but if this is a new kind of logic, perhaps that should be applied in a different PR (?)
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.
Is it different? Engines and prisma cli version are known to cli for sure and for @prisma/client
version we actually try to require it in a similar way I am suggesting
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.
prisma -v
currently shows the actually installed versions, not just the content of package.json
.
// `wrangler` is the only underlying driver that can be just a `devDependency`. | ||
if (underlyingDriverVersion === null && underlyingDriver === 'wrangler') { | ||
underlyingDriverVersion = pkgJson.devDependencies?.[underlyingDriver] ?? null | ||
} |
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.
We should look in dependencies
too because people will sometimes put it there as well
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.
In fact we look into dependencies
just a couple of lines earlier :)
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.
Right, then I would say the opposite 😄
In general this makes sense.
But it's possible that people misplace the adapter packages and place them in devDependencies
, and we should look in both places without assuming they would only be for sure in "devDependencies" or "dependencies".
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.
TLDR: we better not have any assumption on where any packages are placed
* E.g., `@prisma/adapter-planetscale` -> `@planetscale/database` | ||
*/ | ||
|
||
export const underlyingDriverAdaptersMap = { |
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.
I think this module would make more sense as a part of cli
, not driver-adapter-utils
:
cli
is the only one using it and I don't see that changing anytime soon- it's weird that generic library with building blocks and abastractions for specific drivers now has a list of all specific adapters hardcoded.
This PR closes https://github.com/prisma/team-orm/issues/379.
It modifies
prisma -v
so that:@prisma/adapter-*
) Driver Adapters' versions are read and displayedDriver Adapters are not shown when using the binary Query Engine.
Consider the following
package.json
:Then,
prisma -v
would now display:Notes:
@prisma/adapter-not-recognized
is not a valid / well-known Driver Adapter, so it's not included in the output.@prisma/adapter-planetscale
is found, but its underlying driver,@planetscale/database
, isn't. That's because it's not installed inpackage.json
.@prisma/adapter-pg
is found, but its underlying driver,pg
, isn't. That's because it doesn't appear independencies
.@prisma/adapter-d1
is found, and its underlying driver,wrangler
, is also found. This is the only case in which the underlying driver can be retrieved fromdevDependencies
(@prisma/client
needs these drivers at runtime).