fix(drizzle): reimplement Drizzle adapter #10479
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
@realmikesolo is attempting to deploy a commit to the authjs Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
adapters
Got it. I will revert to the previous table names |
| "mysql2": "^3.2.0", | ||
| "postgres": "^3.3.4", | ||
| "tsx": "^4.7.0" | ||
| } | ||
| } |
|
👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎ This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
|
@ndom91 we're ready for review and to be merged! |
|
wow, thanks a lot for the contribution @realmikesolo @AlexBlokh , we'll look into it! cc @ndom91 |
| @@ -19,7 +19,8 @@ | |||
| "exports": { | |||
| ".": { | |||
| "types": "./index.d.ts", | |||
| "import": "./index.js" | |||
| "import": "./index.js", | |||
| "require": "./index.js" | |||
There was a problem hiding this comment.
I think we can drop this require conditional exports, as we export exclusively for use with ESM and have no intention of supporting CommonJS consumers. cc @balazsorban44
There was a problem hiding this comment.
True, all Auth.js packages are ESM-only
There was a problem hiding this comment.
The require line was added to ensure that the default tables imported from the adapter-drizzle package can be used with the drizzle-kit commands, such as generate and push. These commands are used for applying changes to the database. Without it, users will encounter a no exports main defined error when attempting to use drizzle-kit commands.
We would like to have feature parity of Auth.js when hosted either on something like Vercel or in Docker. Moving the docker config to the same example app we host on Vercel and deploying it via Docker will make this easier to verify
|
Thank you for this PR, I was literally just looking into this myself to do the same thing and wondered why there was hard-coded tables without any schema passthrough. Awesome work! |
|
@realmikesolo could you take a look at the |
|
seems like we're good |
|
Merging shortly 🙏 |
| async createUser(data: Omit<AdapterUser, "id">) { | ||
| const id = randomUUID() |
There was a problem hiding this comment.
could we change this so we can leverage drizzle's .defaultFn() here and let users have control of the id generation?
There was a problem hiding this comment.
you have for the sessions table so don't see why users table should be any different
There was a problem hiding this comment.
There was a problem hiding this comment.
Hi, @juliusmarminge. As MySQL doesn't have returning clause, generated id was used to find the row after insertion. But you are right, we can find inserted row by email for user and by sessionToken for session and remove redundant id generation in the code. Thank you for these suggestions!
There was a problem hiding this comment.
@realmikesolo will you remove these redundant ID's in your follow up PR as well?
| * id: text("id").primaryKey().$defaultFn(() => randomUUID()) | ||
| * sessionToken: text("sessionToken").notNull().unique(), | ||
| * userId: text("userId") | ||
| * .notNull() | ||
| * .references(() => users.id, { onDelete: "cascade" }), | ||
| * expires: integer("expires", { mode: "timestamp_ms" }).notNull(), | ||
| * }) | ||
| * }, (table) => ({ | ||
| * userIdIdx: index('Session_userId_index').on(table.userId) | ||
| * })) |
There was a problem hiding this comment.
this is also a breaking change for the underlying database - why was this made?
There was a problem hiding this comment.
Agree with you. It was a mistake to add indexes for foreign keys. I will remove them in new PR
There was a problem hiding this comment.
@realmikesolo thanks for stepping up to remove those, feel free to tag me in the PR so we can get it merged quickly 🙏
|
Amazing! Great direction this is going! :) Would be possible to make the column names also customizable?
Thinking the same thing about the field type. |
|
@realmikesolo thanks for commenting and offering to fix a few of these issues, theres some discussion on this adapter / these issues in the discord as well if you'd like to join:
Also we merged the new docs while this PR was being worked on, the docs are now at |
|
Thank you @ndom91! |
Reasoning
The previous Drizzle adapter for NextAuth was implemented incorrectly. I have rewritten it from the ground up to fix the issues and align it with Drizzle's best practices. The adapter passed all tests. Documentation was updated according to new functionality.
🧢 Checklist