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

Ma/drizzle #571

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Ma/drizzle #571

wants to merge 35 commits into from

Conversation

markusahlstrand
Copy link

Add adapters for drizzle with sqlite and mysql.

@markusahlstrand markusahlstrand marked this pull request as draft March 18, 2024 08:16
@danielgent
Copy link

This PR would be much more clear if we didn't have the adapters

Rather than hundreds of new files, instead there would be inline diffs where we've changed certain calls from kyseley to drizzle

It's not reviewable though with so many changes. I can easily see us missing fixes because files were copy-pasted before we made some critical change. Maybe our tests will cover everything though so we just go for it

Ideally we'd have diffs showing the change though. What about editing the kysely adapters to actually be the drizzle ones and do the rename in another PR?

@danielgent
Copy link

I suppose you've done the hero effort now though so I'd say just merge it and we see if there are errors in production 😄

Lots of this code is copy-pasted (if not most of it?) so it doesn't make sense to review it, maybe you could go through it on a screenshare

@markusahlstrand
Copy link
Author

It's getting closer but not quite done yet.. Need to get the last tests passing and then I would like to see if I can share a big chunk of the code between the sqlite adapter and the mysql adapter, mostly to make sure we run as similar code in both versions as possible. Maybe this weekend...
In a next step we can start to update the database model so it fits better to how our entities look.

@markusahlstrand markusahlstrand marked this pull request as ready for review March 31, 2024 12:45
@markusahlstrand
Copy link
Author

Ok, so this switch to use drizzle for the integration tests and for the local environment, but I haven't yet replaced kysely for planetscale. Should be a single line though..

So, one tricky part here is that Typescript can't really work with generics for the different drizzle-adaptors as the functions on the sqlite/mysql functions are different (even though they look the same..). So, the best solution I found is to generate the mysql adapter based on sqlite adapter. Not super elegant but it works..

@@ -19,6 +18,7 @@
"type-check": "tsc",
"pull-translations": "i18nexus pull -k EWI52U0e8j13AX31kLcS-A --path ./src/locales && prettier --write ./src/locales/**/*.json",
"knip": "knip",
"drizzle:generate:sqlite": "drizzle-kit generate:sqlite --schema ./drizzle-sqlite/schema.ts",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be executed to create a sql migration based on the schema file

@@ -0,0 +1,48 @@
#!/bin/bash
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the script that generates the mysql adapter based on the sqlite adapter. Crude, but works..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool name! Can we change Sesamy to this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there meant to be so much SQL in Git? No problem with me but I've committed huge MySQL dumps to git before 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what's used to do the diffs so.. You can always generate new migrations but guess it will be from a clean database then. Guess new files will be incremental changes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are the names coming from? Verging on NSFW here! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants