Conversation
✅ Deploy Preview for orchid-orm canceled.
|
79ab48c to
769149c
Compare
packages/pqb/src/adapters/bun-sql.ts
Outdated
| result: unknown, | ||
| query: RecordUnknown, | ||
| ): QueryResult<T> => { | ||
| // Bun SQL returns exotic objects; shallow-copy to normalize. |
There was a problem hiding this comment.
Copying rows should be avoided if possible, and I think it should be possible to keep the rows, even if they're exotic in some way.
packages/pqb/src/adapters/bun-sql.ts
Outdated
|
|
||
| if (arrays) { | ||
| const arrayRows = await query.values(); | ||
| const rowCountRaw = meta.count ?? meta.rowCount; |
There was a problem hiding this comment.
this is suspicious: why would there be either count or rowCount, looks like something isn't right.
There was a problem hiding this comment.
Agreed, fixed. (Reworked the whole parser)
|
Thank you for your efforts! PR looks good overall. But also it looks like a good beginning, but not is not there yet. I'm personally not interested in Bun because for me it only adds additional scope of bug fixes, and it's one additional thing to think about whenever adding changes that might need some Bun-specific additions. If you're ready to drive it - great! But are you sure? This will need more time. I expect there to be bugs. Could you share your motivation, why to take additional risks and invest time, if node.js just works? For me, it's an opportunity:
For me Bun is an opportunity to have more appealing benchmarks, but only after SQL optimizations are done and benchmarks are implemented, so it's too early for Bun. I have just one strong requirement to merge Bun: - name: Run Bun adapter tests
run: |
(cd packages/pqb && bunx --bun jest bun-sql)
(cd packages/orm && bunx --bun jest bun-sql)
(cd packages/rake-db && bunx --bun jest bun-sql)The I don't do that for node-postgres, because node-postgres was default for most of the time I know it works, now postgres-js is default and I test everything with it. Whenever changing something adapter-specific I temporary switch Bun is different, it shouldn't be trusted, no idea if all the tests can pass using it, and what can suddenly break, so CI should run all the tests over Bun. |
|
Also, there are few ideas to add to existing adapters.
Here my point is that the adapters are going to be changed in a following couple of month. Do you really want Bun sooner than later? If not really, I think the best would be to postpone it for a several months. |
Let's separate:
Bun as runtimeI'm investing into this because despite having build dozens of projects with TypeScript over past 10 years and experimenting with different runners and bundlers, I wasn't able to come up with a satisfying way to create production builds from my TypeScript monorepos. Somehow I always end up with huge babel (then vite) configs full of hacks and fragile pipelines (or use Nuxt/Nitro monoliths which work surprisingly well but scale poorly from the architectural standpoint). I recently decided I would give Bun a try, and started 2 new projects with it to see how it goes. It's actually my 3rd approach through past years; the 0.x Bun immediately repulsed me with bugs being all over (e.g. oven-sh/bun#4035 or oven-sh/bun#4039). Today it became noticeably better. Bun.SQLNo particular reason to invest here tbh — technically Postgres.js works fine in Bun. That's just a fun vibe code experiment (or I thought so — I ended up reworking half of the AI slop anyway; I missed some spots like with
No, I’m not in desperate need of it, but at the same time, if it works and it's well-tested, then why not? Besides, I think it’s a relatively shallow wrapper, it just handles executing queries and running transactions — and if it does that correctly, I don’t see much potential for bugs. Most of ORM/query builder code works on different level anyway.
Agreed; actually, at first I created a GitHub Actions matrix to test Node and Bun fully, but then when I started to decompose the Publish step, and it turned out it relied on artifacts generating during tests (for the coverage step). I haven't had energy to pursue this fully so ended up with half-baked testing approach. |
Then yes, sure, if you have time and energy to complete the testing approach. Totally up to you, because, again, for me Bun is something that is good to have in the future, but I'd postpone it for a several months.
If all tests can pass on Bun, or if it's easy to fix what is failing then yes, the bun is well-cooked. I didn't try it, if you're ready to drive it further please make the CI changes to run all tests on Bun and it will be clear that yes, Bun is really good now, or if it's a headache.
Those artifacts are needed for the test coverage badge that appears to be broken again - that's not very important. |
769149c to
901a608
Compare
|
The adapter code is much better and has more test coverage now; I'm yet to add the test matrix. |
See #601, this adds support for the native Bun.SQL adapter.
The initial boilerplate was vibe coded, but then heavily cross-reviewed between Codex and Claude and nit-picked by me.
Not saying it should be merged right away, but that's a reasonably good draft I believe.