search

nextauthjs / next-auth

Authentication for the Web.
https://authjs.dev
ISC License
24.31k stars 3.39k forks source link

Support Async SQLite Clients with DrizzleORM #8335

Closed AsyncBanana closed 9 months ago

AsyncBanana commented 1 year ago

Description 📓

Currently, Auth.js' DrizzleORM SQLite adapter does not support Async SQLite clients like the Turso/LibSQL client or the Cloudflare D1 client. This is due to the queries returning promises, which are not properly dealt with

I would be happy to submit a PR to fix this, as most of the code would be almost exactly the same as with the synchronous SQLite client. However, I am not sure how to go about implementing it. Would it make the most sense to create an entirely separate "subadapter" like the ones for MySQL and Postgres, or is there a better way to implement this?

How to reproduce ☕️

Simply create an Auth.js project with the Drizzle adapter and use an asynchronous SQLite driver (in my case, the Turso/LibSQL driver). Try logging in, logging out, and logging in again. The second time you should see Auth.js trying to create a duplicate OAuth account because the query in getUserByAccount returns a promise, which is not resolved.

Contributing 🙌🏽

Yes, I am willing to help implement this feature in a PR

techmunk commented 1 year ago

Seems like https://github.com/techmunk/next-auth/commit/cb6b2a8a61fc8549efe73d2299a8d6b130c466fd is all that would be needed.

I don't really have time to go back and forth on a PR, so feel free to take this and make one. It seems to work for me and the tests pass... Not sure if there's any other issues the nextauth team would flag before merging.

AsyncBanana commented 1 year ago

Simply making the sqlite adapter async is an option I explored. However, I do not think it is a great idea due to the performance cost for synchronous sqlite drivers (promises do have an overhead).

techmunk commented 1 year ago

They can have a cost of course, however in this case I don't think it would change much as:

  1. The callstack into the adapter is already Promise driven.
  2. If an awaited promise is actually synchronous, then it's resolved in that same iteration of the event loop anyway, so the cost is really only the extra function wrapping that async/await/Promise driven code adds.

In any event, not really my call to make. Hopefully someone from the team chimes in and answers.

For now I'm just going ahead with a "patch-package" patch. Progress never stops right? Or something like that. :)

AsyncBanana commented 1 year ago

Also, I realized most functions could be left alone anyways due to them not dealing with the promise values and therefore the caller handling the promise in the response, so most functions wouldn't carry any cost.

gediminastub commented 12 months ago

Any news on this? Thanks!

jasongitmail commented 11 months ago

Same, blocked on this

adidoes commented 10 months ago

bump, blocked by this too

emekaorji commented 10 months ago

Bump, any updates here? Thanks.