-
Notifications
You must be signed in to change notification settings - Fork 0
feat: basic Drizzle ORM setup #24
Conversation
|
Tested the setup on macOS (Apple Silicon, Node 24.20, pnpm 12.8). It works, and seeding completed. Some notes from following the backend README:
Adding cd backend at the top of the guide would help, and make it clear that all the following commands will be done inside /backend
A short explanation of what .env is, plus the exact command, would make it clearer: cp .env.example .env |
|
This was found with the use of AI, and the comment is AI generated: While testing I checked how empty values are stored, since csv-parse returns empty strings for empty columns. From backend/, open the database: pnpm db:psqlThen run: SELECT count(*) AS totalt,
count(*) FILTER (WHERE series = '') AS tomme,
count(*) FILTER (WHERE series IS NULL) AS null_verdier
FROM books;Result: All 52,478 books were imported, but 29,008 of them (55%) have an empty string instead of NULL in series, and none are NULL. The other nullable columns (language, publisher, edition, etc.) are probably the same. Search works today because of this: searchBooks always adds ILIKE '%...%' on every column, and '' matches '%%'. If the seed is changed to store NULL, those books would disappear from every search, since NULL ILIKE '%%' is not true in SQL. Suggestion: store NULL for empty values in the seed, and only add a condition in searchBooks when the argument is actually set. Both should be changed together. Not urgent for delivery 2. |
Only filter those columns we actually care about
|
Thanks for pointing out these issues! The documentation has been improved. The seeding script now actually adds NULL values as expected. Here's the same output after the fix: |
|
Backend works well after the changes, but main's frontend is broken after merging #21 and #22. When main was merged into the express branch, the new shadcn files stayed at the root (app/components/ui, app/lib/utils.ts, components.json) instead of moving to frontend/. frontend/app/root.tsx imports ~/components/ui/Toast, which doesn't exist in frontend/, so pnpm dev fails. I can make a quick fix PR that moves them, get them into main, and then you can update this branch with that change before we merge. Holding off the approval until then, since I need to re-approve anyways. |
|
PR #29 is now ready for approval. As soon as that is approved and merged, this PR can move forward. |
|
#29 has been merged. Merged main into this branch as well. I think everything is okay now |
|
Yup, tested frontend. Works fine. Good job, approving. |
The following has changed:
I will need a lot of help here to test if the setup works for all platforms.
Furthermore, please let me know whether any of the instructions are vague or unclear.
I understand this PR adds a lot of tech at the same time, so please feel free to ask questions about how this will be used in the app! The frontend does not call our API yet, so that will need to be done in another PR.
Note: The base branch is the one used in #23 so it's easier to read the changes and it will be changed to
mainonce that PR is merged.This PR should be merged after #23.