lucia-auth / lucia

Authentication, simple and clean
https://lucia-auth.com
MIT License
8.42k stars 449 forks source link

PlanetScaleAdapter should expect a `Client` instance instead of `Connection` #1459

Closed dihmeetree closed 4 months ago

dihmeetree commented 4 months ago

It has been recently recommended by the PlanetScale team that a Client instance be used instead of Connection to make calls to the database.

You can see the comment here on the Drizzle ORM issue here https://github.com/drizzle-team/drizzle-orm/issues/1743#issuecomment-1892634033 which highlights the reason for this change.

If you check the Drizzle Docs https://orm.drizzle.team/docs/get-started-mysql, they have already updated it with the change from:

// OLD
// Create the connection
const connection = connect({
  host: process.env["DATABASE_HOST"],
  username: process.env["DATABASE_USERNAME"],
  password: process.env["DATABASE_PASSWORD"],
});

to

// NEW
// Create a client
const client = new Client({
  host: process.env["DATABASE_HOST"],
  username: process.env["DATABASE_USERNAME"],
  password: process.env["DATABASE_PASSWORD"],
});

I've gone ahead and replaced all instances of Connection with Client to reflect this change.

pilcrowOnPaper commented 4 months ago

Why not both?

dihmeetree commented 4 months ago

Why not both?

I don't think we should be allowing the use of Connection, as it causes unwanted issues (especially around transactions). You can see an example of this here https://github.com/drizzle-team/drizzle-orm/issues/1743. I think enforcing the use of Client (as the Drizzle team is now) should be the way forward. They'll get an type error in their IDE sure, but imo that's beneficial as they'll either have to fix it, or have to explicitly add a ts-ignore or ts-expect-error on their own.

pilcrowOnPaper commented 4 months ago

That's a user-side problem, not the library's. It shouldn't be up to the adapter to enforce it.

dihmeetree commented 4 months ago

That's a user-side problem, not the library's. It shouldn't be up to the adapter to enforce it.

No worries. I've gone ahead and added Connection back in 👍🏻

pilcrowOnPaper commented 4 months ago

Can you update the docs for PlanetScale as well?

skyf0l commented 4 months ago

Need this for migration too

pilcrowOnPaper commented 4 months ago

Thanks!