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

Support Async SQLite Clients with DrizzleORM #8335

Closed
AsyncBanana opened this issue Aug 16, 2023 · 8 comments · Fixed by #9355
Closed

Support Async SQLite Clients with DrizzleORM #8335

AsyncBanana opened this issue Aug 16, 2023 · 8 comments · Fixed by #9355
Labels
enhancement New feature or request triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime.

Comments

@AsyncBanana
Copy link

AsyncBanana commented Aug 16, 2023

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

@AsyncBanana AsyncBanana added enhancement New feature or request triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Aug 16, 2023
@techmunk
Copy link

Seems like techmunk@cb6b2a8 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
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link

Any news on this? Thanks!

@jasongitmail
Copy link

Same, blocked on this

@adidoes
Copy link

adidoes commented Nov 8, 2023

bump, blocked by this too

@emekaorji
Copy link

Bump, any updates here? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime.
Projects
None yet
6 participants