Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Cloudflare Hyperdrive to enable Postgres and MySQL database connectivity alongside the existing D1 (SQLite) support. The changes expand the database options available to users of the better-auth-cloudflare plugin.
- Introduces generic
DrizzleConfigtype to support multiple database types - Adds validation to ensure only one database configuration is provided at a time
- Updates documentation with Hyperdrive usage examples and configuration details
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/types.ts | Refactors database configuration to support Postgres, MySQL, and D1 through a generic DrizzleConfig type |
| src/index.ts | Implements database configuration validation and conditional adapter setup for all three database types |
| README.md | Updates documentation to reflect Hyperdrive support with usage examples for Postgres and MySQL |
Comments suppressed due to low confidence (1)
src/index.ts:198
- [nitpick] The variable name
databaseis ambiguous in this context. Consider renaming it todatabaseAdapterordrizzleAdapterto clarify that it holds the adapter instance.
let database;
|
@imjlk - curious what you think, can you give a review? |
imjlk
left a comment
imjlk
left a comment
There was a problem hiding this comment.
Hello! Thank you for your efforts. I left a few comments, please take a look when you have time.
No need to worry too much about my feedback. I'm not really used to do review. 😅 Thank you!
| // Assert that only one database configuration is provided | ||
| const dbConfigs = [cloudFlareOptions.postgres, cloudFlareOptions.mysql, cloudFlareOptions.d1].filter(Boolean); | ||
| if (dbConfigs.length > 1) { | ||
| throw new Error( | ||
| "Only one database configuration can be provided. Please provide only one of postgres, mysql, or d1." | ||
| ); | ||
| } | ||
|
|
||
| // Determine which database configuration to use | ||
| let database; | ||
| if (cloudFlareOptions.postgres) { | ||
| database = drizzleAdapter(cloudFlareOptions.postgres.db, { | ||
| provider: "pg", | ||
| ...cloudFlareOptions.postgres.options, | ||
| }); | ||
| } else if (cloudFlareOptions.mysql) { | ||
| database = drizzleAdapter(cloudFlareOptions.mysql.db, { | ||
| provider: "mysql", | ||
| ...cloudFlareOptions.mysql.options, | ||
| }); | ||
| } else if (cloudFlareOptions.d1) { | ||
| database = drizzleAdapter(cloudFlareOptions.d1.db, { | ||
| provider: "sqlite", | ||
| ...cloudFlareOptions.d1.options, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
How about enforce the single database constraint at the type level, instead of using a runtime check?
We can define a type for the database options, like this:
type DatabaseOption =
| {
/**
* D1 database configuration for SQLite
*/
d1: DrizzleConfig<typeof d1Drizzle>;
postgres?: never;
mysql?: never;
}
| {
/**
* Postgres database configuration for Hyperdrive
*/
postgres: DrizzleConfig<typeof postgresDrizzle>;
d1?: never;
mysql?: never;
}
| {
/**
* MySQL database configuration for Hyperdrive
*/
mysql: DrizzleConfig<typeof mysqlDrizzle>;
d1?: never;
postgres?: never;
};By applying this DatabaseOption type to WithCloudflareOptions, we can guarantee that only one database configuration is provided at compile time, eliminating the need for the runtime check.
The final type would look like this:
type WithCloudflareOptions = CloudflarePluginOptions &
Partial<DatabaseOption> & {
/**
* KV namespace for secondary storage, if you want to use that.
*/
kv?: KVNamespace<string>;
};There was a problem hiding this comment.
@imjlk Great feedback! I've been thinking about this too... I wasn't sure which would be clearer to user of the plugin.
There was a problem hiding this comment.
Ah yes, if it's for user feedback, the original method would be better. Also, I did a quick build with the code I wrote and didn't encounter any type errors when even if i set up multiple databases 😞
However, I think it's not common case to use multiple databases
There was a problem hiding this comment.
https://github.com/zpg6/better-auth-cloudflare/pull/9/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R5-R11
Could you change it to add .js at the end, like I did?
I'm not sure if changing the tsconfig settings will solve the issue, but for now, Vite environments are encountering module resolution errors when importing like below:
Error [ERR_MODULE_NOT_FOUND]: Cannot find module './schema' imported from
'./dist/index.js'
I don't think these changes affect the other examples.
If this is applied, I think I’ll be able to update just the example in the SvelteKit example PR after rebasing, without change anything else. I noticed that a type error occurs, but works well.
@imjlk Thank you for taking time and for the great feedback! Will look more into the .js issue too |
Adding support for Cloudflare's Hyperdrive
Closes #6. Inspired by #7 from @imjlk