-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
WIP/POC support prisma 5.4 serverless driver adapters #8847
Conversation
@dcousens I'll rebase this onto the |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 19d623d:
|
@@ -108,6 +108,8 @@ export function createSystem(config: KeystoneConfig) { | |||
adminMeta, | |||
getKeystone: (prismaModule: PrismaModule) => { | |||
const prePrismaClient = new prismaModule.PrismaClient({ | |||
adapter: | |||
typeof config.db.prismaAdapter === 'function' ? config.db.prismaAdapter() : undefined, |
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.
Maybe we should support a () => new prismaModule.PrismaClient(...)
function parameter in db
configurationn, that way users who want the control can have the control by passing their own PrismaClient
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.
If users did that, we could either pass datasources
and log
as parameters to them, or alternatively prevent their configuration through a sum type (that way users would take that responsibility completely).
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.
@dcousens yes that makes totally sense 👍 will do that.
That also makes it possible to update the prisma client so users can use their own prisma client extensions (already tried to do that on a project but only could make it work with the not recommended Prisma Middleware approach)
Updating the PR asap 👍
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.
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 @mmachatschek, I'll probably refine and bike shed, but if you wanted to ensure this is working for your usecases, I'm happy to help push it across the line for the next major.
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.
@dcousens would be great if you could push that across the line. I can assist in any way. I haven't dived into the testing stuff of this repo. Happy to help in any direction
288cf39
to
2d12d6b
Compare
## Example Project - Extend Prisma Schema | ||
|
||
This project implements an example of mutating the Prisma Schema to: | ||
|
||
- Set the Prisma Binary Target; | ||
- Add a multi-column column unique constraint to a list; and | ||
- Add a NOT NULL relationship field | ||
|
||
These show three separate ways of mutating the Prisma Schema see `keystone.ts` and `schema.ts`. | ||
|
||
## Instructions | ||
|
||
To run this project, clone the Keystone repository locally, run `pnpm` at the root of this repository, then navigate to this directory and run: | ||
|
||
```shell | ||
pnpm dev | ||
``` | ||
|
||
This will start the Admin UI at [localhost:3000](http://localhost:3000). | ||
You can use the Admin UI to create items in your database. | ||
|
||
You can also access a GraphQL Playground at [localhost:3000/api/graphql](http://localhost:3000/api/graphql), which allows you to directly run GraphQL queries and mutations. | ||
|
||
Congratulations, you're now up and running with Keystone! 🚀 | ||
|
||
## Try it out in CodeSandbox 🧪 | ||
|
||
You can play with this example online in a web browser using the free [codesandbox.io](https://codesandbox.io/) service. To launch this example, open the URL <https://githubbox.com/keystonejs/keystone/tree/main/examples/extend-prisma-schema>. You can also fork this sandbox to make your own changes. |
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.
This whole directory (custom-prisma-driver-adapter) is the copy of the extend-prisma-schema
example.
This could also be the start of showing an example with prisma client extensions like https://github.com/prisma/extension-read-replicas
would be super beneficial for users
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 am thinking we might use Prisma extensions for hooks in the near future, so it does have me thinking about how this functionality might impact the order of operations there.
As is, if we supported the custom PrismaClient
, Keystone would be .extend
ing the custom client, which I think is ok?
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.
Yes, as far as I know and my testing has gone you call $extend()
on the prisma client and get a new instance of the prisma client back which extends the "old" prisma client instance but doesn't modify it.
The only issue with the current setup of keystone is, that when you call context.prisma.xxxx
you won't have access to the user or keystone defined extensions as the infered type of the prisma client comes from what ever @prisma/client
resolves to.
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 am thinking we might use Prisma extensions for hooks in the near future, so it does have me thinking about how this functionality might impact the order of operations there.
@dcousens I love this idea and would make the usage of context.prisma
over context.db
much easier if you have custom logic in list hooks
idField?: IdFieldConfig; | ||
prismaClientPath?: string; | ||
prismaSchemaPath?: string; | ||
|
||
extendPrismaSchema?: (schema: string) => string; | ||
prismaClient?: (config: PrismaClientConfig) => any; |
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 know that the core doesn't pull in the @prisma/client
for various reasons, I don't know if its desired to have this type 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.
The problem is that some code bases might have many different @prisma/client
's, so you can't rely on that type.
}; | ||
|
||
const prePrismaClient = config.db.prismaClient | ||
? config.db.prismaClient(clientConfig) |
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.
theoretically we could do instanceof prismaModule.PrismaClient
🤔 this should probably have the correct instance of the prisma client which the end user is using
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 don't think we need to do that, we are relying on Typescript in many scenarios, I don't think we need to introduce a run-time burden for this.
I am open to adding something like zod
for the configuration at some point, but, Typescript is the priority.
@@ -107,15 +107,19 @@ export function createSystem(config: KeystoneConfig) { | |||
graphQLSchema, | |||
adminMeta, | |||
getKeystone: (prismaModule: PrismaModule) => { |
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.
This is great to see @mmachatschek thanks!!
I have been playing around with the new Prisma Planetscale Serverless driver with Keystone getContext
. I have got it working here borisno2/on-the-hill-drama-club#189 I wonder is we can also make this usecase easier or less confusing as to which "Prisma Client" would actually be used in that instance. I wonder if we can work towards maybe prismaModule
optional here in getKeystone
and by extension getContext
?
From a DX perspective it feels strange to pass in the Prisma Client twice, and I also found it unnecessarily complex to pass in an customised Prisma Client into getContext
.
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.
Oh that's a good point, getContext
requiring a PrismaModule
is not reasonable with this change
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.
@borisno2 @dcousens I pushed a commit to attempt to remove passing in the prismaModule into the getKeystone
and getContext
functions
We now accept a prismaClientModule
function that returns a PrismaClient
instance and the Prisma
object.
If that function (prismaClientModule
) exists, we can assume that the user passes in a prisma client and don't need to instantiate and import it anymore.
162f51d
to
9fe430c
Compare
@mmachatschek are you interested in rebasing this? |
@dcousens I'm currently unable to polish/rebase this PR to a state where it would be mergeable. Prisma works with 5.11 in our huge codebase. We have tested the prisma serverless adapter which also works. We only needed to add the workaround that @borisno2 added to his project #8847 (comment) You can close the PR if you want to or pick up the changes I made. Feel free to decide how you want to move forward. I'm happy with any decision |
This is a WIP and more like a POC.