Skip to content
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

Split @lucia-auth/adapter-prisma #1152

Closed
wants to merge 14 commits into from
Closed

Split @lucia-auth/adapter-prisma #1152

wants to merge 14 commits into from

Conversation

pilcrowOnPaper
Copy link
Member

This adds 3 new adapters:

  • prisma() to @lucia-auth/adapter-sqlite
  • prisma() to @lucia-auth/adapter-postgresql
  • prisma() to @lucia-auth/adapter-mysql

It also updates the adapter tests to handle bigints

@cloudflare-pages
Copy link

cloudflare-pages bot commented Sep 24, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 07f6b2c
Status: ✅  Deploy successful!
Preview URL: https://0bad05f3.lucia-5ka.pages.dev
Branch Preview URL: https://prisma-split.lucia-5ka.pages.dev

View logs

@tobiasdiez
Copy link
Contributor

May I ask what's the advantage of directly using the underlying db commands over prisma? This would mean you loose a few goodies such as query optimatization, client extensions and other special handling of other fields (say you add another field to a key, then the db type needs to be manually converted to the correct type etc).

@pilcrowOnPaper
Copy link
Member Author

With the current approach, you can't rename schema fields. I'm not sure how client extensions or optimization can be useful here

@tobiasdiez
Copy link
Contributor

Client extensions are useful, since you can do something like validate(...).user.myextension(). This would be no longer possible with these new adapters as user is no longer a prisma user.

If it's really only for being able to rename fields (which I'm not sure is that important), you could add user-specified transformers from the prisma type to what lucia expects, and vice versa.

@pilcrowOnPaper
Copy link
Member Author

Client extensions are useful, since you can do something like validate(...).user.myextension()

I'm still struggling to see how or when that's useful?

If it's really only for being able to rename fields (which I'm not sure is that important)

Didn't you open #1153? Prisma doesn't support renaming fields when using prisma migrate, so we can't just change the Prisma adapter to use camel case. So it's less about having the ability to rename fields and more about getting consistent field names without breaking existing projects. And using snake case for the actual database columns is the de-facto standard anyway.

you could add user-specified transformers from the prisma type to what lucia expects, and vice versa.

I'd rather not. It's just adds more config, and it'll be better if people just build their own adapter

@tobiasdiez
Copy link
Contributor

Client extensions are useful, since you can do something like validate(...).user.myextension()

I'm still struggling to see how or when that's useful?

The canonical example is that you save first and last name in your database, but you want to often use the full name in your code. With extensions you can simply call user.fullname to get the fullname. (Of course there are workarounds but there is a reason why prisma introduced these extension methods).

If it's really only for being able to rename fields (which I'm not sure is that important)

Didn't you open #1153? Prisma doesn't support renaming fields when using prisma migrate, so we can't just change the Prisma adapter to use camel case. So it's less about having the ability to rename fields and more about getting consistent field names without breaking existing projects. And using snake case for the actual database columns is the de-facto standard anyway.

Of course, changing the names would be a breaking change. But afterwards I don't think a lot of users need the flexibility to use different field names - and if they need, they can of course write their own adapter as you said. Btw you do can relatively easily rename columns/fields with prisma migrate: https://www.prisma.io/docs/guides/migrate/developing-with-prisma-migrate/customizing-migrations#example-rename-a-field

@pilcrowOnPaper
Copy link
Member Author

pilcrowOnPaper commented Sep 24, 2023

But afterwards I don't think a lot of users need the flexibility to use different field names

That's the side effect of this change, not the main goal (which is to provide a way to use camelCase for Prisma fields).

Btw you do can relatively easily rename columns/fields with prisma migrate

Manually editing the migration script sounds like a hack

I'd rather use the Prisma interface directly but Lucia's adapters were mostly intended for direct database access, rather than ORMs. This change would make using Prisma more consistent with other adapters and approaches

@tobiasdiez
Copy link
Contributor

Of course, I cannot and don't want to tell you how to maintain this awesome library. But this PR seems like a lot of maintenance burden in the future for relatively little benefit. In my opinion, simply renaming the fields to align with the prisma conventions in new major version would be fine and sufficient.

(For what's worth auth-js is also directly using the prisma client: https://github.com/nextauthjs/next-auth/blob/main/packages/adapter-prisma/src/index.ts)

@pilcrowOnPaper
Copy link
Member Author

pilcrowOnPaper commented Sep 25, 2023

But this PR seems like a lot of maintenance burden in the future for relatively little benefit.

It's actually the opposite. We can ditch a package, and it shares a lot of code with the other SQL adapters.

@tobiasdiez
Copy link
Contributor

Well, you ditch the prisma package for people that use prisma anyway - so not a real benefit for the end user (of course, you don't have to worry about prisma things, that's right).

Would you accept a PR for the existing prisma adapter to change the field names (as breaking change)? Then people can decide on their own if they want to do the hassle with the manual migration or switch to one of the database-specific prisma adapters. What do you think?

@pilcrowOnPaper
Copy link
Member Author

I don't see a difference between ditching the current adapter and introducing a breaking change to the existing one. They both require some sort of migration.

@pilcrowOnPaper
Copy link
Member Author

Closing this since I totally forgot that error handling is going to be more confusing with the change. Using raw queries will throw the SQLite/MySQL/Postgres errors instead of a Prisma error.

@pilcrowOnPaper pilcrowOnPaper deleted the prisma-split branch December 7, 2023 05:40
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants