-
-
Notifications
You must be signed in to change notification settings - Fork 8
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add orchid driver #17
Conversation
I do not implement And I can't setup local test, for my new driver and existing drivers, could you please help me run the test? |
Cool! Thanks a lot for the PR dude. I came across Orchid ORM a while back and thought it looked like Lucid with other some cool features
Yes, bentocache will automatically create the table if
What's the problem? There's nothing in particular to do other than launch the docker-compose in the repo which starts a postgres. then launch the tests. But let me know if anything else is giving you trouble Looks we have some lint issues too |
@Julien-R44 As for @romeerez Hi, does |
@bingtsingw it's very nice to see this PR! I'm seeing this Bentocache library for the first time and I'm not sure yet what exactly is required from a db driver. In some cases, it can be enough to just write a bit of custom SQL to create a table. But looks like in this case, it should happen dynamically. Imagine that OrchidORM has an exported separate function So I'll look into Bentocache to see what should be changed in Orchid to support it. |
We could do a dynamic import of
In fact we don't really have that need in Bentocache. Bentocache just expects a "cache" table with key,value, and expires_at. If you need to change the table, then yes, traditional migrations are recommended. See the knex driver as reference: https://github.com/Julien-R44/bentocache/blob/main/packages/bentocache/src/drivers/database/adapters/knex.ts#L64 |
@romeerez Thank you for your support, in async createTableIfNotExists(): Promise<void> {
await this.#connection.schema
.createTable(this.#tableName)
.addColumn('key', 'varchar(255)', (col) => col.primaryKey().notNull())
.addColumn('value', 'text')
.addColumn('expires_at', 'bigint')
.ifNotExists()
.execute()
} There is no need to change it in this situation. |
@Julien-R44 Thank you for your explain, I thought it was a must. |
@bingtsingw I have looked around, and, well, any ORM is an overkill for this case, even Knex and Kysely are overkill, since Bentocache requires only limited functionality from a driver, it should be easy enough and better for performance to do all queries (to create a table, get value, set value) with raw SQL by calling ORM has a ton of additional functionality that won't be used: relations management, various features like callbacks on create/update/delete, |
@romeerez Yes, in fact ORM is overkill for this tiny cache table, that's why I do not use Knex or Kysely and want to implement an orchid driver, cause I already use it in my project, making it a driver of bentocache is natural. |
Aha, yes that makes sense. In such case, IMO it would be better for Bentocache to have a pg lib adapter, then it could work with any ORM that depends on You could extract pg.Pool from the db instance and the Bentocache pg driver would use it for performing queries directly: const db = orchidORM({ ...options }, { ...tables })
// extract pg.Pool from db instance:
const pgPool = db.$adapter.pool
const value = await pgPool.query('SELECT value FROM cache WHERE key = ?', ['key']) |
Yeah, this is actually something I've hesitated about on bentocache. Do I create adapter for For example, just for Postgres, I would have had to make adapters for I hope I didn't make a mistake 馃槄 |
btw @romeerez, I've just had a closer look, sorry about the comparison with Lucid ORM, Orchid ORM has nothing to do with it. I got it confused with https://sutando.org/guide/getting-started.html which indeed look similar to Lucid Orchid ORM looks really cool, well done for the work !! |
Thank you! It's indeed similar to Lucid, since Lucid is on Knex, and Orchid is syntactically inspired by Knex to be familiar for node.js devs. Lucid's creator posted some interesting notes about reaching a good type-safety level with ORMs, and this is the exact main goal of Orchid ORM to reach. |
@Julien-R44 Hi, I setup the test, seems it fails with |
Weird. I'm not sure why. The tests run fine in CI though: https://github.com/Julien-R44/bentocache/actions/runs/8880648142/job/24383374119?pr=17#step:7:128 Maybe you can ignore the local execution of the knex tests. Just run the tests you added on Orchid. Or even let the CI execute them |
@Julien-R44 Hi, I've completed this pr including |
Awesome. thanks a lot for taking the time to create this PR. I will give a final look at it and merge it before the end of the week |
Some tests did not pass, I took a look at the error messages, not sure if it's related to orchid driver |
We have some flakky test in the suite. It was one of them 馃槃 All green now |
Okay that looks perfect to me. Let's merge it. Thanks again for the PR ! |
Released with bentocache@1.0.0-beta.9 |
No description provided.