-
-
Notifications
You must be signed in to change notification settings - Fork 462
feat: migrate to prisma-orm #434
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
base: main
Are you sure you want to change the base?
Conversation
|
Looking nice! Just had a quick look on the phone but feel free to change the names if you want I dont know anything about prisma really |
|
@C4illin i think it's ready for review now. Some notes:
/// A user of the application
model User {
/// The unique identifier for the user
id Int @id @default(autoincrement())
/// The date and time that the user was created (UTC)
createdUtc DateTime @default(now())
/// The email address of the user
/// @zod.string.email()
email String? @unique
/// The username of the user
/// @zod.string.min(1).max(255)
username String @unique
/// The hashed password of the user. If not set, user can only login via external providers
password String?
/// The display name of the user
/// @zod.string.min(1).max(255)
displayName String
/// The files that the user has uploaded
files File[]
/// The presentations that the user has created
presentations Presentation[]
/// The provider accounts that the user has linked
providerAccounts ProviderAccount[]
}That allows you to have thourough checks on emails and so on and potentially even autogenerate forms.
|
C4illin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
| if (!fs.existsSync("./data/mydb.sqlite")) { | ||
| // run bun prisma migrate deploy with child_process | ||
| console.log("Database not found, creating a new one..."); | ||
| execSync("bun prisma migrate deploy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the standard way to do it?
Not that it is anything wrong with it just want to double check
| .as(User) | ||
| .get(body.email); | ||
| if (existingUser && existingUser.id.toString() !== user.id) { | ||
| const emailInUse = await prisma.user.findUnique({ where: { email: body.email } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! :)
There shouldn't be any users on the old user_version, the update was a long time ago. But I think your current solution can be kept as is just to be sure, there is no harm in it except some technical dept? |
I don't think much validation is needed, the only user input is username and password. And I think it is quite nice being able to have a short or long password and/or username :) but there is no harm in adding it as long as it is backwards compatible |
|
Run |
|
Found a bug in the delete action |
Prisma-Orm migration
This PR aims to improve the dev experience and open the many possibilities that come with an ORM by migrating db-queries to Prisma. At the same time, i will try to optimize the queries a bit for performance.
I personally see this as a prequisite before working on #146, since it allows me to reuse many of my existing authentication utils, and will ensure a conflict-free migration path for existing users.
If you are very opposed to prisma for any reason, please let me know why, and we can surely find an alternative (but i won't do the migration work again ;))
Advantages:
Drawbacks:
TODOs:
provide option to overwrite the db typeout of scope for this PRupdate documentation-> since all migration happens automagically, i don't see the relevance in updating docs for now. Maybe in another PRAdditional notes:
I did adjust the typescript-internal names to fit in with the prisma naming conventions, but kept the original names for compatibility purposes. Please let me know if you do not like this.